:Paolo Amadini Assignee
	Description • 11 years ago

The DeferredTask module currently does not support asynchronous tasks, and this means that it does not cover a use case that is quite frequent now, that is saving data to a file in the background, while ensuring that the operation is robust on shutdown. We have collected several such use cases here:

https://wiki.mozilla.org/Snappy/AsyncShutdown

Since this is required by the work on the Login Manager in bug 853549, I have reworked DeferredTask based on the experience from the Downloads API and the Add-on Manager's "DeferredSave" module.

After examining several in-tree consumers, I went for an approach that should cover a good deal of existing and potential uses of DeferredTask. This module introduces small changes in the behavior of existing consumers, in order to avoid adding too much complexity to the module itself.

Having found no out-of-tree consumers, I went for dropping compatibility and renaming the methods to "arm", "disarm", and "finalize". The old names are ambiguous with an asynchronous task, for example "cancel" does not actually cancel a running task.

DeferredTask is already a good name for the module, that gives an idea of what it does, but since it is not a Task nor a Deferred, we could also rename it to DelayedRunner or something similar, let me know. I don't think it's necessary to keep the old DeferredTask around as conversion is trivial.

I have tested the new consumer in the Downloads API manually, I still have to test the Search Service and the Social Service.

	Dave Townsend [:mossop]
	Comment 1 • 11 years ago

Comment on attachment 8334540 [details] [diff] [review]
The patch

Review of attachment 8334540 [details] [diff] [review]:
-----------------------------------------------------------------

Seems to be missing tests around arming/disarming when an async task is executing.

::: toolkit/components/search/nsSearchService.js
@@ +3871,5 @@
>      engine._initFromMetadata(aName, aIconURL, aAlias, aDescription,
>                               aMethod, aTemplate);
>      this._addEngineToStore(engine);
> +    this.batchTask.disarm();
> +    this.batchTask.arm();

Seems like this is likely to be a common case, should DeferredTask take an option to just make it do this?

::: toolkit/modules/tests/xpcshell/test_DeferredTask.js
@@ +267,5 @@
> +      do_timeout(3*T, () => { timePassed = true; });
> +    }
> +
> +    yield promiseTimeout(1*T);
> + 

Nit: bad indentation

	:Paolo Amadini Assignee
	Comment 2 • 11 years ago

(In reply to Dave Townsend (:Mossop) from comment #1)
> > +    this.batchTask.disarm();
> > +    this.batchTask.arm();
> 
> Seems like this is likely to be a common case, should DeferredTask take an
> option to just make it do this?

This was my initial thought also, but after more investigation it appears that most of the potential consumers don't really need that, thus I opted for less configuration and more predictability of what the "arm" call actually does.

	:Paolo Amadini Assignee
	Comment 3 • 11 years ago

Comment on attachment 8335939 [details] [diff] [review]
Updated patch

Gavin, I've updated the Search Service to use AsyncShutdown to solve the existing issue with asynchronous flushing on shutdown. How can I verify that the new shutdown handling works?

Note that I observed intermittent xpcshell failures in test_serialize_file.js and test_multipleIcons.js, but those seem to be pre-existing.

	David Teller [:Yoric] - still alive but not very active
	Comment 4 • 11 years ago

Looks like this might break some add-ons, won't it?

	:Paolo Amadini Assignee
	Comment 5 • 11 years ago

(In reply to David Rajchenbach Teller [:Yoric] <needinfo? me> from comment #4)
> Looks like this might break some add-ons, won't it?

Yes, we should document the change, even though an MXR search shows that no add-ons are using this module.

	Dave Townsend [:mossop]
	Comment 6 • 11 years ago

Comment on attachment 8335939 [details] [diff] [review]
Updated patch

This looks good. Could you add one more test before landing, that arming and immediately disarming the tesk while it is running asynchronously doesn't cause it to re-run.

	:Gavin Sharp [email: gavin@gavinsharp.com]
	Comment 7 • 11 years ago

(In reply to :Paolo Amadini from comment #3)
> Gavin, I've updated the Search Service to use AsyncShutdown to solve the
> existing issue with asynchronous flushing on shutdown. How can I verify that
> the new shutdown handling works?

The best way to manually verify that the batchTask code works is to make a modification to the engine store (e.g. add an engine with Services.search.addEngine/addEngineWithDetails, change one of its attributes like its alias, reorder it, etc.) and then shut down immediately, and ensure that the changes get properly propagated to search.json before shutdown.

For lazySerializeTask, that's testing the serialization of the actual XML for non-default search plugins when one of its parameters change. So you'd want to addEngineWithDetails and then call addParam a bunch of times, and test that the serialization works (including if you shut down immediately).

We should probably also have some tests that cover these code paths, but search service test coverage is far from ideal :(

	:Paolo Amadini Assignee
	Comment 8 • 11 years ago

I've added the new unit test, and manually tested that the shutdown handling for batchTask in the Search Service works, by lengthening the delay from 1s to 10s.

I've also tested the delayed serialization triggered by addParam. The delay is used only for saving the XML once in case of multiple consecutive calls, and there continues to be no shutdown handling for that. Since it operates on a short delay (100ms), I don't think that adding new code for handling shutdown is needed, even if it is now possible.

	:Irving Reid (No longer working on Firefox)
	Comment 9 • 11 years ago

While building the AsyncSave module, I found that tests in particular are quite likely to try and shut down before delayed saves happen. Also, in the case of the Add-on Manager, there was a condition that triggered an attempt to write during shutdown so it was always losing the race.

If there's any chance that add-ons will use any of these APIs we need them to be shutdown-safe, because we don't want to leave an opening for an add-on to break us by using the API in its shutdown hook.

For testing this sort of async code, one technique I've used is to load the JSM under test directly, and then replace the module-global reference to the async service with a mock that the test driver controls (see for example the mock AsyncShutdown in toolkit/mozapps/extensions/test/xpcshell/head_addons.js, added by bug 925389, or the mock OS.File.writeAtomic in .../test_DeferredSave.js)

	:Paolo Amadini Assignee
	Comment 10 • 11 years ago

(In reply to :Irving Reid from comment #9)

These are good points, and thanks for the unit testing suggestions! I've filed bug 945629.

	:Paolo Amadini Assignee
	Comment 11 • 11 years ago

https://hg.mozilla.org/integration/fx-team/rev/295b75fac0c8

	Carsten Book [:Tomcat]
	Comment 12 • 11 years ago

had to backout this change in https://hg.mozilla.org/integration/fx-team/rev/b4284cae2bff because of test failures after this checkin like https://tbpl.mozilla.org/php/getParsedLog.php?id=31373611&tree=Fx-Team

	:Paolo Amadini Assignee
	Comment 13 • 11 years ago

Re-landed this bug only, since this is most probably unrelated to the reported B2G failures:

https://hg.mozilla.org/integration/fx-team/rev/d1e4fed327fe

	Ryan VanderMeulen [:RyanVM]
	Comment 14 • 11 years ago

https://hg.mozilla.org/mozilla-central/rev/d1e4fed327fe