Closed Bug 1332024 Opened 3 years ago Closed 3 years ago

Add a method to explicitly finalize `JSONFile` objects

Categories

(Toolkit :: General, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox54 --- fixed

People

(Reporter: Lina, Assigned: Lina)

Details

Attachments

(2 files, 1 obsolete file)

Comment on attachment 8828035 [details]
Bug 1332024 - Add a method to explicitly finalize `JSONFile` objects.

https://reviewboard.mozilla.org/r/105564/#review106386

I don't have time to review this now so redirecting to paolo.

::: toolkit/modules/JSONFile.jsm:301
(Diff revision 1)
> +    this._finalized = true;
> +    yield this._saver.finalize();
> +    this._data = null;
> +    this.dataReady = false;
> +  }),
> +  _finalized: false,

Please put this at the top of JSONFile.prototype with the other properties
Attachment #8828035 - Flags: review?(MattN+bmo)
Comment on attachment 8828035 [details]
Bug 1332024 - Add a method to explicitly finalize `JSONFile` objects.

https://reviewboard.mozilla.org/r/105564/#review106456

::: services/sync/tests/unit/test_telemetry.js:71
(Diff revision 1)
> +  // Bug 1325523: we create new engine and tracker instances for each test.
> +  // Since the tracker persists in the background, it's possible for items
> +  // tracked in one test to survive until the next test. This causes
> +  // intermittent failures, so we explicitly finalize storage between tests
> +  // to make sure all our changes are persisted.
> +  await engine._tracker._storage.finalize();

are we confident that other tests aren't going to be hit by this? I'm wondering if a finalize call should be in engineManager.unregister() or similar?
Comment on attachment 8828035 [details]
Bug 1332024 - Add a method to explicitly finalize `JSONFile` objects.

https://reviewboard.mozilla.org/r/105564/#review106632

Good timing, I was just discussing this yesterday in bug 1287660 comment 27 and later! The patch itself has timing issues though ;-)

I was a bit undecided if we needed this as a production-accessible method. It's tricky to implement and use properly, and in most cases if you want to open and also close a JSON file in production code, in other words use the object without a service lifetime, you shouldn't use JSONFile at all, just write the file with OS.File methods.

I can still see rare use cases like rotating a file name though, and maybe there are use cases I didn't think about, for example in Session Restore. To avoid footguns when trying to implement those, maybe we really need a proper "JSONFile.finalize()" method.

Two main issues with the current patch:
1) We really need to "removeBlocker", or blockers will accumulate over time if this is constructed a lot in production code. Consumers of an object with a "finalize" method expect it to be fully cleaned up when the method is called.
2) Possible race condition on shutdown in production code:
- You call "finalize" manually in production code.
- The file write starts.
- The browser starts to shut down.
- The shutdown blocker gets called, but it's a no-op.
- The browser completes shutdown, file write is interrupted.

This may seem a little bit nitpicky, but we are driving towards a more robust browser, and we should ensure the underlying components are as robust as we can get them. While Session Restore doesn't use JSONFile.finalize() currently, it might be used one day for any reason, and issues of this kind DO happen when you have millions of shutdowns every day.

Another less important but still relevant issue is that consumers must be diligent, and we should allow "finalize" to be called only one time. Further calls should throw exceptions, just like DeferredTask does. This behavior in DeferredTask has been helpful to catch shutdown issues with this already!

I guess we need a more nuanced internal finalization method that returns the same Promise instance to both the public "finalize" method and the shutdown blocker. When this promise is resolved, the public "finalize" method can remove the shutdown blocker.

Remember that we need to add a specific JSONFile regression test for the behavior of the new method, including when it's expected to throw exceptions. Race conditions are difficult to test, if there isn't an easy way I think we can avoid writing those tests for the moment.

Thanks for working on this!
Attachment #8828035 - Flags: review?(paolo.mozmail)
Attachment #8828035 - Attachment is obsolete: true
Thank you for the thorough feedback, Paolo! I made the changes you requested: moving finalization into an internal method that returns the same promise to the public `finalize` method and the blocker, having `finalize` remove the blocker, and only allowing `finalize` to be called once. I also added a couple of tests for explicit and automatic finalization. Please take another look.
Comment on attachment 8829950 [details]
Bug 1332024 - Add a method to explicitly finalize `JSONFile` objects.

https://reviewboard.mozilla.org/r/106900/#review108016

Wonderful! I like the way you solved the shutdown barrier testing.

While reviewing the tests I noticed a new possible race condition that I didn't originally notice when looking at the code, but it really required to activate an extra nitpicking mode...

::: toolkit/modules/JSONFile.jsm:263
(Diff revision 1)
>    /**
>     * Called when the data changed, this triggers asynchronous serialization.
>     */
>    saveSoon() {
> +    // Lazily register a shutdown blocker the first time we save. We keep a
> +    // reference to the bound finalization function so that `finalize` can
> +    // remove the blocker.
> +    if (!this._finalizeInternalBound) {
> +      this._finalizeInternalBound = this._finalizeInternal.bind(this);
> +      this._finalizeAt.addBlocker("JSON store: writing data",
> +                               this._finalizeInternalBound);
> +    }
>      return this._saver.arm();
>    },

If you never call saveSoon(), then call finalize(), then call saveSoon(), the last call will throw, but it will leave the finalization blocker registered.

::: toolkit/modules/JSONFile.jsm:272
(Diff revision 1)
> +      this._finalizeAt.addBlocker("JSON store: writing data",
> +                               this._finalizeInternalBound);

nit: three more spaces needed for correct alignment.

::: toolkit/modules/JSONFile.jsm:304
(Diff revision 1)
>     */
>    _processLoadedData(data) {
>      this.data = this._dataPostProcessor ? this._dataPostProcessor(data) : data;
>    },
> +
> +  _finalizeInternal() {

nit: we may add a short JSDoc like the other internal methods.

::: toolkit/modules/tests/xpcshell/test_JSONFile.js:320
(Diff revision 1)
> +  yield storeForSave.finalize();
> +  yield rejects(storeForSave.finalize(), /has already been finalized$/);

For a more thorough test, call "finalize" again immediately, without waiting on the returned Promise first. I believe the code handles this correctly already.

::: toolkit/modules/tests/xpcshell/test_JSONFile.js:321
(Diff revision 1)
> +  yield storeForSave.load();
> +  storeForSave.data = TEST_DATA;
> +  storeForSave.saveSoon();
> +
> +  yield storeForSave.finalize();
> +  yield rejects(storeForSave.finalize(), /has already been finalized$/);

Please name the Assert module explicitly like the rest of the file does. While some reviewers encourage reducing the number of characters, just writing the function name makes it difficult to understand if it's a local helper or a standard check function.

The well-known "ok" and "is" functions can be exceptions, but the rest of the module uses the "do_check_" syntax so you might as well use the same here for consistency.

::: toolkit/modules/tests/xpcshell/test_JSONFile.js:324
(Diff revision 1)
> +  // Finalization removes the blocker, so waiting should not throw even though
> +  // the object has been explicitly finalized.
> +  yield barrier.wait();

This comment is slightly incorrect because the blocker shouldn't throw anyways.

I'm not sure if we have a way to test that the blocker has actually been removed, because this is not observable anyways, and has no functional effects apart from the accumulation in memory. If not, I'm fine with leaving the presence of the blocker untested. We've reviewed the code anyways.

::: toolkit/modules/tests/xpcshell/test_JSONFile.js:349
(Diff revision 1)
> +  // Arm the saver, then simulate shutdown and ensure the file is
> +  // automatically finalized.
> +  storeForSave.saveSoon();
> +
> +  yield barrier.wait();
> +  yield rejects(storeForSave.finalize(), /has already been finalized$/);

This is not necessarily wanted behavior, as callers of "finalize" may be unaware of the timing of the shutdown blocker.

However, I'm not sure we should add extra state just to check whether the public "finalize" method has been called, unless it's also needed to solve the issue mentioned at the beginning of the review. A shutdown console error won't cause issues anyways.

Anyways, if you end up keeping this check, add a comment noting that this behavior can be safely changed.
Attachment #8829950 - Flags: review?(paolo.mozmail)
Comment on attachment 8829950 [details]
Bug 1332024 - Add a method to explicitly finalize `JSONFile` objects.

https://reviewboard.mozilla.org/r/106900/#review108016

Thanks for the detailed review!

> If you never call saveSoon(), then call finalize(), then call saveSoon(), the last call will throw, but it will leave the finalization blocker registered.

Whoops, I made the shutdown blocker registration lazy before adding `finalizeAt`. Now that the option exists, I think we can move this back to the constructor.

> Please name the Assert module explicitly like the rest of the file does. While some reviewers encourage reducing the number of characters, just writing the function name makes it difficult to understand if it's a local helper or a standard check function.
> 
> The well-known "ok" and "is" functions can be exceptions, but the rest of the module uses the "do_check_" syntax so you might as well use the same here for consistency.

Fixed and switched to `do_check_*`.

> This comment is slightly incorrect because the blocker shouldn't throw anyways.
> 
> I'm not sure if we have a way to test that the blocker has actually been removed, because this is not observable anyways, and has no functional effects apart from the accumulation in memory. If not, I'm fine with leaving the presence of the blocker untested. We've reviewed the code anyways.

Something like `do_check_false(barrier._conditionToPromise.has(storeForSave._finalizeInternalBound))` before waiting would work, but that's probably excessive.

> This is not necessarily wanted behavior, as callers of "finalize" may be unaware of the timing of the shutdown blocker.
> 
> However, I'm not sure we should add extra state just to check whether the public "finalize" method has been called, unless it's also needed to solve the issue mentioned at the beginning of the review. A shutdown console error won't cause issues anyways.
> 
> Anyways, if you end up keeping this check, add a comment noting that this behavior can be safely changed.

Comment added.
Comment on attachment 8828036 [details]
Bug 1332024 - Finalize tracker storage and consolidate cleanup logic in engine tests.

https://reviewboard.mozilla.org/r/105566/#review108110
Attachment #8828036 - Flags: review?(markh) → review+
Comment on attachment 8829950 [details]
Bug 1332024 - Add a method to explicitly finalize `JSONFile` objects.

https://reviewboard.mozilla.org/r/106900/#review108262

Looks great, thanks!

::: toolkit/modules/tests/xpcshell/test_JSONFile.js:323
(Diff revision 2)
> +  storeForSave.saveSoon();
> +
> +  let promiseFinalize = storeForSave.finalize();
> +  yield Assert.rejects(storeForSave.finalize(), /has already been finalized$/);
> +  yield promiseFinalize;
> +  do_check_true(!storeForSave.dataReady);

nit: do_check_false, also below.
Attachment #8829950 - Flags: review?(paolo.mozmail) → review+
I found an interesting race in `test_populateFieldValues.js`: we finalize the object after `test_populateFieldValues_with_invalid_guid` exits, but before `load` finishes. We can avoid that by checking if we've already finalized the object in `_processLoadedData`; it doesn't really make sense to process and set the data if we're shutting down.
Pushed by kcambridge@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/53e6a592327b
Add a method to explicitly finalize `JSONFile` objects. r=Paolo
https://hg.mozilla.org/integration/autoland/rev/7b546022d4ac
Finalize tracker storage and consolidate cleanup logic in engine tests. r=markh
https://hg.mozilla.org/mozilla-central/rev/53e6a592327b
https://hg.mozilla.org/mozilla-central/rev/7b546022d4ac
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
No longer blocks: 1325523
Duplicate of this bug: 1325523
You need to log in before you can comment on or make changes to this bug.