[#2060] drop exceptions thrown by @Every jobs so that they get rescheduled

[davidcostanzo]

This pull request fixes a regression from 1.2.7 that was introduced by the fix for [#1518]. In short, the way that "every" jobs are scheduled will cause them to stop being re-scheduled if they throw an exception. The fix for [#1518] started to throw exceptions that had previously been swallowed by the invocation framework. The fix for [#2060] is to swallow any exceptions in the narrow context between an "every" job and the executor service. The fix for [#1518] is retained.

The ticket for [#2060] only mentions the @Every annotation, but Job.every() had the same regression. To ensure that the fix was in one place, some refactoring was done to make the @Every logic invoke Job.every(). A slight (and pleasant) side-effect of the refactoring is that /@status no longer reports @Every("never") jobs as being scheduled to "run every never".

The pull request includes unit tests, which pass on 1.2.7, fail on 1.4.3, and pass with the fix.

[asolntsev]

@xael-fry Seems reasonable. I suggest to merge this PR.

[flybyray]

Generaly summary: users need mentoring using the Play!-Job-API. The javadoc on onException is not explicit // Customize Invocation.

But he is right the re-throw in onException must be a conditional one. That commit, should have introduced the flag within now or a special now(boolean throwExecutionException):
45d390d#diff-34df15bef2557dbc31eb399824919cfeR138

[flybyray]

first try to avoid anonymous class wrapper creation if you can just use a flag: https://gist.github.com/flybyray/a7d61c66cd666c8aa391b3a0fb92425a#file-job-java-L168
https://gist.github.com/flybyray/a7d61c66cd666c8aa391b3a0fb92425a#file-job-java-L182

[davidcostanzo]

I also dislike anonymous class wrappers. The reason I didn't add a new flag to the Job class is that it's not a property of the job, but how the job was scheduled. I didn't want to introduce hard-to-understand failures when a Job is scheduled multiple times by the application. For example, if someone wanted to run a Job now and every 24 hours afterward, they could write something like:

  Job j = MyJob();
  j.every("24h");
  try {
    await(j.now());
  } catch (Exception ex) {
    error("could not run the job");
  }

If Job.every() sets a flag in the Job object, then putting j.every() before the j.now() would interfere with the exception propagation, but the reverse wouldn't. To me, that seemed counter-intuitive. The current Job implementation uses the anonymous wrapper class created in Job.getJobCallingCallable() to manage the exceptions for jobs scheduled by Job.now() and Job.in() without reading/writing state in the Job, so I followed suit.

That said, the applications written by my organization never the above pattern, so we wouldn't be negatively impacted by keeping a "scheduled by every" flag within the Job object. So if that's the way you want me to go, that's the way I'll go. Please confirm.

I suppose you recommended making this behavior controllable by a new configuration option so that someone could disable it if they wanted an unhandled exception to halt the "every" execution. Are there additional documentation requirements for introducing a new configuration option, or would it be acceptable to keep it as a hidden option, available to anyone who reads the source code?

[flybyray]

That commit, should have introduced the flag within now or a special now(boolean throwExecutionException):
45d390d#diff-34df15bef2557dbc31eb399824919cfeR138

my suggestion is:

• fix just the commit 45d390d on line 138 which introduced the error (by using a flag, check now and in too)
• for your own problems the use of onException should be sufficient

Or am I missing something? Overriding onException is the way to go if you want to ignore errors or special error handling.

[davidcostanzo]

fix just the commit 45d390d on line 138 which introduced the error (by using a flag, check now and in too)

Thanks for the extra clarification. I'll rework my pull request to do this, following the pattern of your Gist, but applied to now() and in() instead of every(). It may take me a while to put the update together.

[flybyray]

override is easy! why not just override onException too?

[davidcostanzo]

I don't understand the recommendation. This unit test is designed to test what happens when someone doesn't override onException (the common case, and the case cited in [#2060]). If it overrides onException, then it would be testing something else. Could you sketch out a bit more what you had in mind?

[davidcostanzo]

Thank you for the thoughtful review, @flybyray. I don't know if your "general summary" comment was directed at me, but you're correct that I never considered Job.onException() to be part of the public API. I don't know why, but I always considered it to be Play! internals.

I'm sorry to be a pest, but I didn't understand all of the recommendations, so I responded with questions instead of a code update. Once I get a clear understanding of what you have in mind, I'll be able to fix my broken pull request.

[flybyray]

@davidcostanzo it was directed to all who cannot understand what // Customize Invocation. means. the javadoc comment is messy. Should be fixed too! And at all the documentation could be better with a topic on exception handling for jobs https://www.playframework.com/documentation/1.4.x/jobs

[davidcostanzo]

I have updated the pull request as @flybyray recommended, adding a flag to limit the exception throwing to the code paths of any job scheduled by in or now. The pull request includes setting this flag for afterRequest, so that afterRequest's return value has consistent behavior, regardless of whether the job was scheduled with now or as an after action.

Units tests were added to cover the exception handling of in, now, and @On.

The pull request update also reverts the change to JobsPlugin.java, since Job.every() remains simple enough not to warrant this refactoring.