Clojure

Converted from http://www.assembla.com/spaces/clojure/tickets/390
Attachments:
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

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

richhickey said: This needs careful analysis

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.

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)
  @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.

stu said: [file:cVNaMc15Wr35VzeJe5cbLr]

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

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

Test appears to failing because of CLJ-672

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.