java.lang.InterruptedException race in chiming functions.

[dazld]

There seems to be a race between closing a chime and the chiming function invocation that can lead to a java.lang.InterruptedException being thrown when functions are awaiting something coming from, for example, a future, or have consumed a function that does this somewhere along the call stack.

It'd be nice if either the chiming function was never called, or any inflight functions were allowed to complete before closing.

Example:

(let [now (Instant/now)
      chiming (chime/chime-at (chime/periodic-seq now (Duration/ofSeconds 1))
                              (fn [time]
                                (log/info :future-task @(future (count {})))
                                (log/info :chime (str time)))
                              {:error-handler (fn [e]
                                                (log/error :chime-failed e)
                                                true)
                               :on-finished #(log/info :done)})]
  (Thread/sleep 1000)
  (.close chiming))

outputs:

2020-09-07 15:51:25.548 INFO  :future-task 0
2020-09-07 15:51:25.549 INFO  :chime 2020-09-07T14:51:25.547924Z
2020-09-07 15:51:26.550 INFO  :done
2020-09-07 15:51:26.550 ERROR :chime-failed #error {
 :cause nil
 :via
 [{:type java.lang.InterruptedException
   :message nil
   :at [java.util.concurrent.FutureTask awaitDone FutureTask.java 418]}]
 :trace
 [[java.util.concurrent.FutureTask awaitDone FutureTask.java 418]
  [java.util.concurrent.FutureTask get FutureTask.java 190]
  [clojure.core$deref_future invokeStatic core.clj 2300]
  [clojure.core$future_call$reify__8454 deref core.clj 6974]
  [clojure.core$deref invokeStatic core.clj 2320]
  [clojure.core$deref invoke core.clj 2306]
  ...
  [chime.core$chime_at$schedule_loop__13568$task__13572$fn__13573 invoke core.clj 91]
  [chime.core$chime_at$schedule_loop__13568$task__13572 invoke core.clj 90]
  [clojure.lang.AFn run AFn.java 22]
  [java.util.concurrent.Executors$RunnableAdapter call Executors.java 515]
  [java.util.concurrent.FutureTask run FutureTask.java 264]
  [java.util.concurrent.ScheduledThreadPoolExecutor$ScheduledFutureTask run ScheduledThreadPoolExecutor.java 304]
  [java.util.concurrent.ThreadPoolExecutor runWorker ThreadPoolExecutor.java 1128]
  [java.util.concurrent.ThreadPoolExecutor$Worker run ThreadPoolExecutor.java 628]
  [java.lang.Thread run Thread.java 834]]}

Note, might have to run this a couple of times to see the exception, as it doesn't always happen.

[jarohen]

Thanks @dazld, will have a look 😄

[dazld]

No biggie! Was taking chime for a test drive and these couple of things popped out. While we’re talking, would you mind pushing a release of the version with the IPending implementation? That threw me as well, and noticed it’s fixed in master.

[jarohen]

Ah, I see what's going on here - I'm afraid this is (currently, at least) intended behaviour within Chime. If a schedule is explicitly closed, we assume the system is shutting down in some way, and interrupt the scheduling thread to give it a chance to cleanly close any resources it's opened. Interrupting a thread that's currently waiting on a deref causes the deref to throw the InterruptedException.

With that in mind, we can repro it reliably by making the future (Thread/sleep 500), say.

One example of prior art here would be the difference between ExecutorService's shutdown and shutdownNow methods - Chime behaves like the latter. I'll preserve this for close, at least, to avoid breaking users that depend on it.

I'll consider what it'd involve to add something like that to Chime, and what it's impact would be. In the meantime, maybe either a lock or a shared continue? flag would achieve the same aim?

(let [now (Instant/now)
      close-lock (Object.)
      chiming (chime/chime-at (chime/periodic-seq now (Duration/ofSeconds 1))
                              (fn [time]
                                (log/info :chime (str time))
                                (locking close-lock
                                  (log/info :future-task @(future (Thread/sleep 500)))))
                              {:error-handler (fn [e]
                                                (log/error :chime-failed e)
                                                true)
                               :on-finished #(log/info :done)})]
  (Thread/sleep 250)
  (locking close-lock
    (.close chiming)))

Re the 0.3.3 release - I've pushed a 0.3.3-SNAPSHOT up with the other fixes.

Thanks again for raising this issue :)

[jimpil]

The behaviour of .shutdownNow() (as seen from consumers) can be simulated with a cancelled? (AtomicBoolean) flag, can't it? Instead of calling (f time) true, you would call (when (false? (.get cancelled?)) (f time) true). Doesn't this make moving to .shutdown() possible? Yes, it would leave the task scheduled for one more time, but that time will not actually run. I'd say that's a sensible compromise in order to get rid of the racy nature of Interrupts. Just a thought...

[jimpil]

BTW, I tried out the approach described in my previous comment, and the only test that breaks is this (for obvious reasons):

FAIL in (test-cancelling-overrunning-task) (core_test.clj:111)
expected: (instance? InterruptedException (clojure.core/deref !error))
  actual: nil

Ran 18 tests containing 45 assertions.
1 failures, 0 errors.
Tests failed.

[jimpil]

A complementary approach would be to let consumers choose how the executor is shutdown (i.e. :orderly VS :abruptly), defaulting to the latter for backwards-compatibility. Here is a sketch:

(let [pool (Executors/newSingleThreadScheduledExecutor thread-factory)
      !latch (promise)
      cancelled? (atom false)
      shutdown* (case exec-shutdown 
                     :orderly (fn [] 
                                (reset! cancelled? true)
                                (.shutdown pool))
                     :abruptly (fn [] (.shutdownNow pool)))] ;; default

[jimpil]

I retract my last two comments! There is already a !latch that can be leveraged:

(when-not (realized? !latch)
   (f time)
   true)

This allows moving to .shutdown() w/o introducing anything new, and w/o breaking anyone. The only 'breaking' (if you want to call it that) change is that custom error-handlers no longer have to mimic the default one (wrt to checking for the InterruptException) - something which, to my surprise, is not documented. Isn't it a problem if a provided error-handler ignores the interrupt and returns true (in the current interrupt-based approach)?

[EDIT]: Actually it's not 'breaking', is it? The default error-handler (or any other that already mimics it) can stay as is - they just will never see an InterruptedException, so that check is effectively redundant (functionally equivalent to true). In the absence of interrupts altogether, the overall behaviour of chime doesn't change - does it?

[jimpil]

I'm trying to digest the following comment (seen a few messages above):

One example of prior art here would be the difference between ExecutorService's shutdown and shutdownNow methods - Chime behaves like the latter. I'll preserve this for close, at least, to avoid breaking users that depend on it.

This is an implementation detail, and users shouldn't depend on it - in fact I'm struggling to see how they even could. Users of chime come to it looking for a flexible scheduling primitive. The promise/guarantee of chime can be exhausted in single sentence - your function will be called at the times you provided, unless you manually cancel (and that's very powerful btw). As long as chime can uphold that promise, what constructs you use to implement it is sort of irrelevant. Users that depend on esoteric intricacies of some implementation should know better, but in the case of chime, with such a narrow API I honestly don't see how users can possibly be depending on the difference of shutdown VS shutdownNow(). The only thing that (indirectly) cares about it is the default error-handler which special-cases InterruptedException (somewhat sneakily I might add, as I can't find any mentions that this type of exception should be given any form of special attention - say by custom error handlers).

My point is that chime can keep its exact same api/functionality, while slightly changing its implementation, which today is centered around a non-deterministic/best-effort/racy interrupt-based approach (all because of shutdownNow()), and nobody will be able to tell the difference.

Looking back at the history of the project, I can see that !latch is a kind of a recent addition (as an attempt to make sure that the on-finished callback is called only once). This means that in its conception the code didn't have this notion of are-we-done? that the !latch later brought. In a world where you don't have this explicit notion, the best you can do is play with interrupts and hope for the best. In that context, I can totally see why the original implementation started out as it did (based on interrupts). It's actually pretty neat/clever if you think what it had to work with. However, you later did add the !latch and that gives you all the control you need to move away from interrupts, while keeping (I'd argue more reliably) the don't call f after cancel semantics that users care about.

Anyway, enough waffling...i think it's pretty clear how much I want to use chime... 👍

[jarohen]

Hey @jimpil, thanks for the analysis - you've clearly gone quite deep into this 😄

If I may, to summarise, it seems there's a couple of points here:

1. "Users may want both shutdown and shutdownNow behaviour." Indeed, I recall reading that this was the reason that ExecutorService doesn't implement Closeable - because there's no good default here. Maybe we should expose both? I do think that interrupts are the right thing to use here, in the shutdownNow case, given this is how ExecutorServices work - but we could of course be clearer about it.
2. "The default error handler knows to handle InterruptedException specifically, which is arguably too much knowledge" - agreed. I think you've pretty much arrived at the correct cause here - the error handler has been extracted out of what Chime did internally before, and has hung on to the previous implementation detail. I wouldn't be averse to a new handler here (deprecating the old one) with clearer semantics - maybe something closer to the underlying thread's 'uncaught exception handler' would be good prior art to lean on.

Have I understood it correctly? :)

James

[jimpil]

Hi @jarohen,

See my comments inline:

Users may want both shutdown and shutdownNow behaviour

We may arrive at this conclusion in the end, but it is kind of premature to consider this at this stage. We first need to understand the following:

1. What is the public promise of chime?
2. How is that achieved (i.e. what is shutdownNow contributing)?
3. Is it an absolute guarantee, or is it more like a best-effort approach?
4. Are there any alternatives that don't compromise the promise?

Assuming that we agree on chime's singular promise (i.e. I shall call your function at the specified times, until you manually cancel), it's an interesting exercise to go through the rest.

2. How is that achieved (i.e. what is shutdownNow contributing)?

As I've already explained, when there is no global notion of done?, the only way you can implement the until you manually cancel part is via interrupts. Once you accept/embrace this compromise, then of course InterruptedException has to be promoted (so to speak), as that has effectively become the done? signal. In other words, the implementation makes perfect sense, assuming that interrupts are the only viable approach.

3. Is it an absolute guarantee, or is it more like a best-effort approach?

I think you'll agree that relying on interrupts is inherently racy, and therefore by definition a best-effort approach

4. Are there any alternatives that don't compromise the promise?

There hasn't always been one, but the moment you added the !latch you've essentially introduced a global notion of done? (a great name for the !latch btw). Guess what - you no longer need interrupts - you have your own, fully reliable way of communicating when things should stop. This alone, enables you to move to shutdown w/o lying (in the sense of not upholding your promise) to anyone.

Now, if after doing all that, you still feel that there are masochists out there who would actually prefer their core scheduling primitive to be racy/not-deterministic and centered around interrupts, despite chime never having made such a promise, and despite virtually no reason to go down that route, then OK sure perhaps you can consider exposing a racy shutdown option, and make it an official feature (or bug depending on your perspective). This is up to your discretion and taste. However, having a non-racy default (which remember, upholds the core promise) is a no-brainer.

I do think that interrupts are the right thing to use here, in the shutdownNow case, given this is how ExecutorServices work - but we could of course be clearer about it.

Yes indeed, if you go down the shutdownNow route you literally have no choice. Being clearer, and instructing users to always keep an eye out for InterruptedException in their error-handlers is super important, given the current implementation. I bet there are plenty of error-handlers out there that don't pay any attention to InterruptedException, but frankly I don't blame them. You can't expect everyone to have the time to go through the source code of every single lib they're using.

"The default error handler knows to handle InterruptedException specifically, which is arguably too much knowledge" - agreed.

Yes but this is kind of necessary. The real problem, given the current implementation is that it's not communicated to the users (e.g via the doc-string - it just mentions truthy VS falsy).

I wouldn't be averse to a new handler here (deprecating the old one) with clearer semantics - maybe something closer to the underlying thread's 'uncaught exception handler' would be good prior art to lean on.

There is no better/clearer error-handler given the current implementation. It will ultimately be an InterruptedException that is thrown (via shutdownNow). Whether you handle that via a global exception-handler (or not), doesn't change the fact that it is a racy interrupt. I would advise taking a step back, and asking yourself why does it need to be this way? (i.e. centered around interrupts)? And if there is indeed a reason that it has to be this way, how come it's not reflected in the tests (i.e. all tests pass when I switch from shutdownNow to shutdown)?

Hope that helps in your thinking process 👍

[jarohen]

Thanks

An important meta point, first of all:

Now, if after doing all that, you still feel that there are masochists out there who would actually prefer their core scheduling primitive to be racy/not-deterministic and centered around interrupts, despite chime never having made such a promise, and despite virtually no reason to go down that route, then OK sure perhaps you can consider exposing a racy shutdown option, and make it an official feature (or bug depending on your perspective). This is up to your discretion and taste. However, having a non-racy default (which remember, upholds the core promise) is a no-brainer.

Please don't dismiss people with alternative opinions in this way on my repos. You have very valid points and suggestions that I'm more than willing to consider - there is no need to detract from them by using such language and tone.

[jimpil]

Please don't dismiss people with alternative opinions in this way on my repos. You have very valid points and suggestions that I'm more than willing to consider - there is no need to detract from them by using such language and tone.

You're right - I apologise for my poor choice of words, but I'm not actually dismissing anyone. I'm basically concluding that there may ultimately be value in accommodating those people (regardless of their opinions, or how they might look to me). By the way, I'm not a native speaker, but I totally get your point...I re-read my comment and i could have easily expressed the same point using different words. Point taken 👍

[jarohen]

I think you'll agree that relying on interrupts is inherently racy

I disagree, I'm afraid. Could you elaborate?

There hasn't always been one, but the moment you added the !latch you've essentially introduced a global notion of done?

Guess what - you no longer need interrupts - you have your own, fully reliable way of communicating when things should stop.

I disagree with these too - that we have a 'global notion of done?', that we have a 'fully reliable means of communication without interrupts', and hence that we don't need them. What happens if the user wants to shut down the schedule while the job is currently running?

Openly, I'm a little discouraged by your outright dismissal of interrupts without an acknowledgement of under what circumstances they might be useful. It doesn't fill me with confidence that you've fully considered the benefits of the opposite position before rejecting it. Similarly, I am aware of their pitfalls - so please don't assume I introduced them without due thought and consideration, nor that I am not sufficiently experienced that I don't know of alternatives.

We may arrive at this conclusion in the end, but it is kind of premature to consider this at this stage. We first need to understand the following:
...
Assuming that we agree on chime's singular promise (i.e. I shall call your function at the specified times, until you manually cancel), it's an interesting exercise to go through the rest.

Likewise. I have been working on/with Chime for ~7 years now. While I don't pretend to have gotten everything right, please do consider that I may have put some thought into it over that time! :)

[jimpil]

I disagree, I'm afraid. Could you elaborate?

The perils of the interrupt flag, what/how affects it, and the general pains of using it correctly have been discussed extensively, and I'd like to avoid focusing on that, as it's rather secondary to my main point. Their most useful feature is the fact that blocking operations like network IO, .sleep(), .wait() etc (things out of our control) attempt to abort/cleanup when they see one. But how does that relate to chime? Does chime aspire to attempt to abort a task that is happening 'right-now' (i.e. has already started)? Which brings me to my next point...

I disagree with these too - that we have a 'global notion of done?', that we have a 'fully reliable means of communication without interrupts', and hence that we don't need them. What happens if the user wants to shut down the schedule while the job is currently running?

A user who wants to stop a side-effecting task that has already started (e.g sending an email, writing a file etc), is by definition a user who doesn't care about how much of the side-effect actually happened. I know it seems like the opposite because he/she (seems to) wants for things to stop asap, but there is no way for them to actually know, unless they are the ones driving it. Moreover, what does it mean to stop the schedule for consumers? Are we not talking about the schedule as a whole (as opposed to a single scheduled task)? If so, then what happens in the real world? What happens when you try to cancel a recurring direct-debit on the same day (let alone same instant) it's supposed to go out? I don't know about your bank, but with mine what gets cancelled is the next instalment. What happens with Amazon when you try to cancel a subscription that has already been processed (a few days before it actually arrives), or after a parcel has been dispatched? If i remember correctly, in the latter case the option to cancel is not even there anymore - you have to go the refund route. Generally speaking, there is a cutoff point with these things, after which it simply doesn't make sense to cancel. In chime's case, that cutoff moment can/should(?) be considered to be the moment something starts to run. Anything else opens the door to potential leaks, corrupted-files, half-sent or abandoned http requests, emails that reached half their usual recipients, or god knows what else - things that should generally be avoided. If you want for user-code to be able to bail-out (say from a loop) when the thread is interrupted, that is kind of a different discussion, to which I'd say that the notions of closing VS cancelling should probably be distinct.

Openly, I'm a little discouraged by your outright dismissal of interrupts without an acknowledgement of under what circumstances they might be useful. It doesn't fill me with confidence that you've fully considered the benefits of the opposite position before rejecting it.

It's rather sad to hear this, as it proves that I didn't quite get the message across. I'm not opposing, nor dismissing interrupts in the general sense. I just don't think they provide a sound foundation to carry the unscheduling semantics of close. They are a great candidate for the cancelling/interrupting notion i mentioned above, but firstly that should be driven ideally only by user-code, and secondly it should be a distinct thing (feature if you will). As far as the benefits of the opposite position go, I was the first to admit/point-out that up until the (recent) addition of !latch, relying on shutdownNow was not just desirable, but also necessary as w/o it you would always have one more side-effect produced after un-scheduling (and that's clearly a problem).

Likewise. I have been working on/with Chime for ~7 years now. While I don't pretend to have gotten everything right, please do consider that I may have put some thought into it over that time! :)

I don't know what idea you got, but I consider the work you've done with chime simply excellent - from the core idea down to the actual implementation, and ultimately I am utterly grateful for the hours you've put in. The discussion we're having here is not to have a go at you, or your work. Quite the contrary I would say - I'm itching to use chime for serious work, and as a consequence I want to understand it and improve it (if possible of course). I am more than happy to be enlightened on anything you feel is important.

Finally, I was already wondering in the back of my head, but given how much more you've re-enforced your position around interrupts and shutdownNow, I just have to ask with a straight face, otherwise I won't sleep tonight :(. How is it possible that such a design center-piece can be swapped out w/o any (real) tests breaking?

Thanks again...

[jimpil]

It just occurred to me that the reify returned could implement Future :

Future
 (cancel [_ interrupt?]
   (when-let [^Future fut @current] ;; `current` is an atom which holds the current-job's future (as returned by `.schecule`)
      (and (not (.isCancelled fut)) 
           (.cancel fut interrupt?))))

Again, just a thought - I'll come back to this with a clean head tomorrow and re-evaluate...