Feature Single-id to handle jobs that should only run once

[guavabot]

This resolves path#51 and addresses part of what was requested in #36 .

When building a job, you can pass a singleId Param, so that if another Job with the same singleId is already queued, they will only run once. This avoids having to manually use static atomic variables or other kinds of locks. It also improves from what was suggested in path#51 in that onAdded() will always be called, so you can do database operations in the JobManager's Executor (if using addJobInBackground). The feature covers the KEEP_OTHER scenario.

Some details:

• onAdded() will still be called on every Job so that you can update the local database if necessary.
• The ID returned from addJob() will be the same for both (or more) jobs.
• Finally, if the previous Job with the same singleId is already running, this behavior will not apply and both jobs will be run normally.

[yigit]

Thanks a lot for the PR. I have not yet carefully reviewed it yet but there is an unfortunate problem :/.
I'm in the middle of re-writing the core in a message based architecture to get rid of the synchronization hell inside.
https://github.com/yigit/android-priority-jobqueue/commits/message-queue
https://github.com/yigit/android-priority-jobqueue/wiki/JobManager-2.0---Roadmap

This will simplify the flow control within the JobQueue. Current architecture makes it really hard to add new features while avoiding race conditions.
I'm also taking this opportunity to re-think some parts of the API (breaking changes) one of which is getting rid of the jobId that is not even unique within the JobManager (it is only unique within its owner queue). I'm mostly done with new implementation but I still have follow-up tasks like changing JobQueue API to allow more fields (maybe even custom constraints).

If we merge this now, I won't be able to rebase 2.0 because it touches many files.
Do you mind waiting until 2.0 and re-considering this there? I can still make a review on current implementation to ensure we are on the same page.

Thanks a lot and sorry for this inconvenient issue.

[guavabot]

Yes, please review the current implementation and let me know what you think.

Any ETA on the 2.0 (at least to the point where I can re-implement this on it) or any part of it where you would use some help?

[yigit]

Thanks for understanding, I'll take a look at this.
The first part of 2.0 is almost ready (re-writing internals to be message queue based). A few tests are acting flaky, I'm trying to figure out whether it is a bug or something wrong with the test.
After that step is complete, I'm going to change JobQueue API to be queried by some constraint POJO instead of parameters. You can implement this feature after that and (hopefully) should be a lot simpler because you wont have to deal with synchronization hell :).

I'll ping this thread once it is ready.

Btw, about this implementation, have you tried implementing it on top of tags/groups instead of adding a new field? I've not thought about this thoroughly so maybe not feasible but I think it should be.

[guavabot]

It should be feasible with the tags. I was thinking of adding a tag with a prefix like "job-queue-single-id:userSelectedId". Maybe leaving the singleId field in Params and adding the tag in the Job constructor. Both ways seem OK to me.

Another thing I was considering was adding a new onAdded method and deprecating the original (like shouldReRunOnThrowable). This method would receive some JobInfo as parameter and return a RunConstraint. Based on the JobInfo, the client could decide if they want onRun to be called or if they want a previous job not to be run. That would require more discussion on what the JobInfo should include. Maybe the id, tags, and delayUntilNs for all Jobs in the same group? Do you think it's worth it to explore that route.

[yigit]

I think it is fine to keep a field in the Params object but I was thinking that we can use the already existing JobQueue#tag API to store the information internally.
Maybe something like

singleInstanceByTag(theTag). I think this should also set the group ID automatically if it is null.

I think user should also clarify the behavior when there is a job running vs waiting + do we cancel the previous job or the new one. If the behavior is to cancel the existing job, then JobManager has to wait until that running job resolves to something before adding the new one otherwise we may add the new one and right after if the app crashes, both jobs will stay in the database (if they are persistent). Maybe, for simplicity, we can start with saying that the new job will not be added if there is an existing one.

Also, is there a particular reason why you are still calling onAdded on the new job even if there is an existing one? I don't have strong opinion on it but I wanted to know if there is a use case (like dispatching a loading event)

[yigit]

already done in the message-queue branch

[guavabot]

Yes, that's what I was trying to say with keeping the field in the Params.

The use case that I was thinking of when I decided to always call onAdded was that you might be editing some resource while offline and you only need the last change to be sent. For example, let's say that you are editing the profile name with a Job like this:

class PostNameJob extends Job {
  final String name;
  PostNameJob(String name) { ....requireNetwork()... }
  public void onAdded() {
    database.saveName(name);
  }
  public void onRun() {
    String name = database.getName();
    restApi.postName(name);
  }
}

As you can see, this would make sure that many changes can be stored locally, and once there's internet connection, the latest change will go to the server. I'm thinking now that maybe it would make sense to add an onBeforeAdded() method for this, called just before inserting into the queue. Then onAdded would only be called if it was really added.

Maybe, for simplicity, we can start with saying that the new job will not be added if there is an existing one.

I was thinking that if the previous job is already running, it is safer to err on the side of adding it anyways. Consider the example above of PostNameJob. If we run the job twice, we waste some bandwidth, but it's not such a big deal, whereas not making the request is a big problem. It would be better to wait until the job resolves to something, as you said, but my "simplicity" approach was to add it anyways. I assume that the jobs where you would use the single instance are the kind of jobs where it's not a big deal if it runs twice (like PostNameJob or like FetchTweetsJob), but maybe you have a different use case in mind.

[guavabot]

OK for all your line notes

[yigit]

Hmm I agree on the use case, we should definitely support it. A case like this will require waiting for the running job to finish or cancel though, otherwise, it may create unexpected inconsistencies because onAdded of the new Job and onRun of the previous job may run in parallel and onRun job may read inconsistent data, which might be a problem. Probably not but it is very hidden for a developer to detect.

Actually, the right way to implement that job would be reading the parameters from the Job not database.
If the Application crashes right after saving the job into database but before running onRun, it will never have the database updated. Even without a singleInstance case, that job may loose data.

Maybe we should consider a special job to handle these cases that takes care of such synchronization issues. Think about it (i'll do so as well). If we can provide a solid way for these, I think it is worth having a separate API.

Btw, I implemented a first pass on JobQueue constraint change.
dfc3b3f

It removed some optimizations but can be re-added.
Here is the constraint class:
https://github.com/yigit/android-priority-jobqueue/blob/dfc3b3f070fe3b11f26815d78d81f4ee71531824/jobqueue/src/main/java/com/birbit/android/jobqueue/Constraint.java

I'm hoping that queues can implement caching using its uniqueId although in current form it includes the time which will surely invalidate any cache. I'll take a look at making it cache-able later.

Here is a generic implementation for constraints handling in sqlite job queue:

android-priority-jobqueue/jobqueue/src/main/java/com/path/android/jobqueue/persistentQueue/sqlite/SqliteJobQueue.java

Line 288 in dfc3b3f

private Where createWhere(Constraint constraint) {

And here is an in memory implementation:
c73ee21

Lmk what you think.

[yigit]

Ok, here is a proposal (very similar to your suggestion).

In terms of behavior:
When singleInstance job is added,

• If there is a job with the same instance id is in the queue, waiting
  • The newly added job receives onCancel right after onAdded. We are not going to call the queue to add though, we'll be calling onAdded just for consistency.
• If there is a job with the same instance id is currently running:
  • The new job is added into the queue
  • The existing job is marked as cancelled. After running
    • If it runs successfully, it is completed.
    • If it throws an exception, it receives the onCancel call.

I think this is the most complete cycle from the Job's perspective. It runs like a regular job, still receives all callbacks (e.g. onAdded).

In terms of internal implementation, I think we should keep it as a parameter in the Params but internally just set a tag. Maybe we can call it something like singleInstanceTag to make this more clear.

Btw, I've just added query caching to sqlite queue as well so I think it is at a stage where you can add this feature.

[guavabot]

Looks good to me. At first I didn't like the idea of calling onCancel right after onAdded, but you're right that it makes sense for the Job's lifecycle.

I'll do this during the week. Are the tests also good now?

[yigit]

yea, i was not very comfortable w/ that either but we have to call onAdded (in case it is doing database changes) so to complete the lifecycle we need the onCancel.

Btw, we should start sending cancel reason to the onCancel method (so that these jobs can decide whether to revert db or not). There is always a reason which is being passed into the JobManagerCallback#onJobRun. It would be nice if you can take care of it in your CL. Just add onCancel(int reason) which calls onCancel by default. Deprecate onCancel and also add the above two cases into onJobRun types. We should also consider changing JobManagerCallback#onJobCancelled to receive the reason instead of a boolean.

Tests should be fine, only the RetryTest is a bit flaky, i didn't have time to look at it yet.

Thanks.

[guavabot]

I'm more or less done, just missing the "cancel reason", but I won't finish today.

Should I update the PR (will include all your message-queue commits) or make a new one based on message-queue?

[yigit]

It would be better to have it based on message-queue branch (it is like 2.0 branch now).
We can continue the review there. You can also upload whatever you have now so I can take a look at it.
Thanks.

[guavabot]

Closing this since it's been rebased on new branch.