<< Back to previous view

[CLJ-390] sends from agent error-handlers should be allowed Created: 29/Jun/10  Updated: 08/Dec/10  Resolved: 29/Nov/10

Status: Closed
Project: Clojure
Component/s: None
Affects Version/s: None
Fix Version/s: Release 1.3

Type: Defect Priority: Major
Reporter: Chouser Assignee: Chouser
Resolution: Completed Votes: 0
Labels: None

Attachments: Text File 0003-Allows-agent-error-handler-to-send.patch    
Patch: Code and Test
Approval: Ok


Currently if an agent's error-handler sends to any agent, the sent action is
silently thrown away. The send should instead be allowed.

Originally reported here:

Comment by Assembla Importer [ 24/Aug/10 11:57 AM ]

Converted from http://www.assembla.com/spaces/clojure/tickets/390
0001-Allows-agent-error-handler-to-send-successfully.-Ref.patch - https://www.assembla.com/spaces/clojure/documents/d9EEI0G4yr36YneJe5cbLr/download/d9EEI0G4yr36YneJe5cbLr
0390-agent-error-send-plus-test.patch - https://www.assembla.com/spaces/clojure/documents/cVNaMc15Wr35VzeJe5cbLr/download/cVNaMc15Wr35VzeJe5cbLr

Comment by Assembla Importer [ 24/Aug/10 11:57 AM ]

chouser@n01se.net said: [file:d9EEI0G4yr36YneJe5cbLr]: Set nested to null instead of to empty vector before invoking error handler

Comment by Assembla Importer [ 24/Aug/10 11:57 AM ]

richhickey said: This needs careful analysis

Comment by Assembla Importer [ 24/Aug/10 11:57 AM ]

chouser@n01se.net said: When the agent's thread-local "nested" field is non-nil, any sends are queued instead of being sent immediately. This is its normal state when agent actions are being executed.

Another piece of thread-local state is agent which is set to the current agent when an action is being executed. This state is also checked by 'await' so that it can throw if it's called during an agent action.

I can think of two options for allowing sends from agent error handlers:

A. Set "nested" to null before the error handler is invoked, allowing any sends from it to proceed immediately when send is called.

B. Clear "nested" to an empty vector as it is now, but then after invoking the error handler release any newly pending sends queued by the error handler.

This patch implements choice (A). It changes the state of the agent's "nested" field from an empty vector to null for the period of time between when the action has terminated with an error and the time when "nested" is normally set to null (after the next action, if any, has been queued. The things that happen during this time are:

1. the error handler (if any) is invoked
2. the error flag is reset (if error-mode is :continue)
3. the current action is popped off the agent's queue
4. the next action is queued

So this change allows sends from this agent during all of these steps. It does not change agent, so 'await' is still disallowed throughout these steps.

Of the 4 things done during this time period, only step 1 calls any user code in the current thread such that thread-local values would have any impact, and none of the others depend on the value of "nested" at all.

During step 1, the user's error handler will be called with agent true and nested nil, which is combination not normally seen. The reasons that sends from inside actions are normally held until later is prevent them from happening if the action fails to "commit" and update the agent's value, and to promise to users that any nested actions will see the updated agent's value (or a subsequent value). Neither of these reasons apply in this case because the state of the agent will not be updated due to the failed action, so allowing sends to proceed immediately is safe.

Comment by Assembla Importer [ 24/Aug/10 11:57 AM ]

shoover said: For what it's worth, I confirm that this patch works for the situations I've tested. My desired case of having an error handler send to a supervisor agent works great.

I tried to break it by having the handler fn send to agent and await agent, but it all goes pretty much like you described.

;; Sending to *agent* from a handler works fine. Note the outer deref will not
;; always see the value :handled because the action that returned :handled
;; was sent from a different thread (the action thread).
(let [handler (fn [_ ex]
                (send *agent* (fn [_] :handled)))
      a (agent 42 :error-handler handler)]
  (send a (fn [_] (throw (Exception. "bad news"))))
  (await a)

await agent in a handler throws an exception as expected, and it is silently eaten like any other handler exception.

The weirdest case is if you mess up in the handler and send agent an action that awaits agent : you get an infinite loop through the handler (if error mode is :continue). I wouldn't recommend sending to agent in a handler, of course, but it's possible with this patch in the same way it's possible to add a watch that performs another send to the same agent. It'll go all day.

Comment by Assembla Importer [ 24/Aug/10 11:57 AM ]

stu said: [file:cVNaMc15Wr35VzeJe5cbLr]

Comment by Assembla Importer [ 24/Aug/10 11:57 AM ]

stu said: I agree with Chouser's reasoning (in the comments).

Comment by Stuart Halloway [ 05/Nov/10 11:06 AM ]

Heisenbugs! Test no longer passing, don't have time to look at it atm. Moving back from OK to Test.

Comment by Chouser [ 07/Nov/10 5:29 PM ]

Test appears to failing because of CLJ-672

Comment by Chouser [ 08/Nov/10 12:54 AM ]

The test added previously was failing because *agent* wasn't bound in the error handler, per CLJ-672.

This patch (0003-) subsumes the previous patches and adds a similar test that does not rely on *agent* being bound. This would have succeeded all along. After a fix for CLJ-672, both tests pass.

Setting this back to Test, but it will fail unless tested after applying the patch from CLJ-672.

Generated at Sun Dec 21 12:52:36 CST 2014 using JIRA 4.4#649-r158309.