Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Android] Synchronize content retrieval over the bridge #36072

Merged

Conversation

mkevins
Copy link
Contributor

@mkevins mkevins commented Oct 29, 2021

Related PRs:

gutenberg-mobile: wordpress-mobile/gutenberg-mobile#4182
WordPress-Android: wordpress-mobile/WordPress-Android#15508

Description

This synchronizes the content retrieval mechanism for serializing content from the editor, and includes title and content in a single event.

Both getTitle and getContent utilize the same mechanism to retrieve data from the editor within the JS runtime: and event is emitted, and a countdown latch awaits the response on the thread that emitted the event. Since this can be invoked concurrently, a race condition currently exists when serialization time is sufficiently long and a second instantiation of a countdown latch overwrites the latch awaited on an earlier thread. By synchronizing access to the latch, we can prevent overwriting of the latch and resolve the race condition.

Since both of these methods each result in the same response (including both the title and the content), we also combine them into a single event.

The access to the latch is also synchronized in the triggerGetContentInfo which relies on the same mechanism (and event).

How has this been tested?

This has been tested by opening a large post and logging the concurrent jobs triggered by the autosave mechanism as well as user initiated save events, and ensuring that the content saved represents the latest state of the UI (the source of truth).

Steps

  1. Create a post (easier on web) with this content pasted into the code editor
  2. Save the post on web and close it
  3. Open the post on mobile and make some changes
  4. Test the following flows
    1. Allow autosave to occur (after waiting more than 500 milliseconds between edits)
    2. Manually save the post
    3. Exit the editor with autosave queued up (the large post will take several seconds to save)

None of the above scenarios should result in a crash.

Types of changes

Bug fix (non-breaking change which fixes an issue)

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • I've tested my changes with keyboard and screen readers.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
  • I've updated all React Native files affected by any refactorings/renamings in this PR (please manually search all *.native.js files for terms that need renaming or removal).

This synchronizes the content retrieval mechanism for serializing
content from the editor, and includes title and content in a single
event.
Comment on lines 918 to 924
try {
// TODO: we should consider logging when await returns false
mGetContentCountDownLatch.await(10, TimeUnit.SECONDS);
} catch (InterruptedException ie) {
// TODO: this should be either renamed, or invoked when the await returns false.
onGetContentTimeout.onGetContentTimeout(ie);
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe that the intent of the get content timeout callback is not achieved by the current code. I think that onGetContentTimeout was meant to be called when the latch await timeout is exceeded, however, await does not throw in this case. Instead, it merely returns false. I believe this logic should be refactored, or at least renamed, since the current naming is misleading, imo. Also, it may be beneficial to log the cases where the timeout is indeed reached (when await returns false), which I think was the original intent.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've updated this with bc2ab62 👍

This renames the interruption callback and logs when the latch times out
prior to a response from the editor.
This adds logging (resolving some TODOs) and also uses an already
extracted method to check if React context is null.
@mkevins mkevins requested a review from antonis November 4, 2021 05:12
@mkevins
Copy link
Contributor Author

mkevins commented Nov 4, 2021

I've added more logging, and addressed a few "TODO" items.

@mkevins mkevins marked this pull request as ready for review November 4, 2021 05:13
This was missed during an earlier refactor
The getTitle method is no longer used, so we can remove it
Copy link
Member

@antonis antonis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work @mkevins 👍
I tested this on a Pixel 5 (Android 12) and everything worked as expected in my tests. The code changes also look consistent to me 🎉

@mkevins mkevins merged commit 4c30e31 into trunk Nov 7, 2021
@mkevins mkevins deleted the rnmobile/fix/synchronize-content-serialization-over-the-bridge branch November 7, 2021 22:21
@github-actions github-actions bot added this to the Gutenberg 12.0 milestone Nov 7, 2021
@mkevins
Copy link
Contributor Author

mkevins commented Nov 7, 2021

Thank you for reviewing and testing!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Mobile App - i.e. Android or iOS Native mobile impl of the block editor. (Note: used in scripts, ping mobile folks to change)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants