-
Notifications
You must be signed in to change notification settings - Fork 2
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
Fix race condition with registering webview postMessage calls over RPC #1406
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome! Good sleuthing, and nice job with the fix! Very elegant. Thanks for the hard work on it. Handful of comments.
Reviewed 10 of 10 files at r1, all commit messages.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @lyonsil)
src/renderer/components/web-view.component.tsx
line 72 at r1 (raw file):
const postMessageCallback = useCallback( ([webViewNonce, message, targetOrigin]: Parameters<WebViewMessageRequestHandler>) => {
FYI if you want to get rid of the array and use the parameters directly, you can do this:
type StuffType = (first: number, second: number, third: string) => void;
function DoStuff(...[first, second, third]: Parameters<StuffType>) {
console.log(first, second, third);
}
Not necessary; just in case you were thinking about it.
src/renderer/components/web-view.component.tsx
line 93 at r1 (raw file):
type UnsubscriberContainer = { unsub: UnsubscriberAsync | undefined }; useEffect(() => {
registrationPromises
needs to be cleaned up for this web view when the web view is unmounted, right? Can you add a new useEffect
with no dependencies that basically just runs registrationPromises.cleanupPromiseChain
in its deconstructor? Or maybe there's a timing issue with this useEffect
that warrants further consideration.
It seems cleanupPromiseChain
doesn't work if new promises get added after it is run. It seems this useEffect
probably has the ability to add promises after unmount, so this is a timing problem. Can cleanupPromiseChain
be refactored so that, if it determines in the finally
that it is not the current promise, it can be set up to listen to whatever the new promise is and try cleaning up after that one?
src/renderer/components/web-view.component.tsx
line 94 at r1 (raw file):
useEffect(() => { let isMounted = true;
Since this useEffect
has stuff in its dependency array, the component will not necessarily be unmounted when its destructor runs. It will mean the current values are outdated and may mean the component is unmounted. Please rename this variable to avoid the confusion. I usually name variables similar to this oneisCurrent
or something like that. Though this variable means something slightly different - something like "is this request handler current or already registered and needs to be unregistered immediately". Maybe I'd recommend isThisRequestHandlerCurrentOrAlreadyRegisteredAndNeedsToBeUnregisteredImmediately
. Really channel our inner Java gurus.
src/renderer/components/web-view.component.tsx
line 150 at r1 (raw file):
return () => { registrationPromises.addPromise(id, async () => { isMounted = false;
I had trouble parsing through all the isMounted
, unsubContainer.unsub
, and promise stuff here and got it only after a while of concentrated effort. It would be helpful to have a few comments talking through the intentions behind these lines (mostly thinking about 141-151).
For example, it took a long time to figure out that it is appropriate that this line isMounted = false
should indeed be in this async function and not outside it.
lib/platform-bible-utils/src/promise-chaining-map.ts
line 26 at r1 (raw file):
* Creates a new PromiseChainingMap * * @param logger Object with a `warn` method that will be called when a promise rejects
Would be nice to mention that this is console
by default if you make other changes in utils, but maybe not worth it otherwise haha
lib/platform-bible-utils/src/promise-chaining-map.ts
line 40 at r1 (raw file):
* @param promiseFunction Function that returns a promise to add to the chain */ addPromise(key: TKey, promiseFunction: () => Promise<unknown>): void {
I pictured this method by its name as receiving a Promise
, not a promise function. Should this be renamed to something more like runAsyncFunction
or addPromiseFunction
or something? Or maybe just reformat to receive a Promise
?
lib/platform-bible-utils/src/promise-chaining-map.ts
line 42 at r1 (raw file):
addPromise(key: TKey, promiseFunction: () => Promise<unknown>): void { const currentPromise = this.map.get(key); this.map.set(key, currentPromise ? currentPromise.then(promiseFunction) : promiseFunction());
Might it be worth adding a .catch
on this .then
that logs a warning with the error? Or maybe add a new parameter of error handler and just log warning by default? Adding an error handler parameter would allow people to decide whether they want to continue the promise chain or throw out of it if there's a problem.
lib/platform-bible-utils/src/promise-chaining-map.ts
line 63 at r1 (raw file):
* @param key Unique key to identify a distinct promise chain */ private cleanupPromiseChain(key: TKey): void {
Maybe worth adding a new parameter of error handler here too? And log by default like it is now
1c0be3e
to
d376628
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 2 of 10 files reviewed, 2 unresolved discussions (waiting on @tjcouch-sil)
lib/platform-bible-utils/src/promise-chaining-map.ts
line 26 at r1 (raw file):
Previously, tjcouch-sil (TJ Couch) wrote…
Would be nice to mention that this is
console
by default if you make other changes in utils, but maybe not worth it otherwise haha
Done
lib/platform-bible-utils/src/promise-chaining-map.ts
line 40 at r1 (raw file):
Previously, tjcouch-sil (TJ Couch) wrote…
I pictured this method by its name as receiving a
Promise
, not a promise function. Should this be renamed to something more likerunAsyncFunction
oraddPromiseFunction
or something? Or maybe just reformat to receive aPromise
?
Renamed to addPromiseFunction
.
lib/platform-bible-utils/src/promise-chaining-map.ts
line 42 at r1 (raw file):
Previously, tjcouch-sil (TJ Couch) wrote…
Might it be worth adding a
.catch
on this.then
that logs a warning with the error? Or maybe add a new parameter of error handler and just log warning by default? Adding an error handler parameter would allow people to decide whether they want to continue the promise chain or throw out of it if there's a problem.
Error handling is done within cleanupPromiseChain()
on the next line. I think starting with a simple implementation that matches the current use case is enough. If something more complex is helpful/useful later, we can always add the functionality at that time and set defaults to match the current behavior.
lib/platform-bible-utils/src/promise-chaining-map.ts
line 63 at r1 (raw file):
Previously, tjcouch-sil (TJ Couch) wrote…
Maybe worth adding a new parameter of error handler here too? And log by default like it is now
Same comment as above - Let's start with the simple case and add complexity as it is needed later (if it ever arises).
src/renderer/components/web-view.component.tsx
line 93 at r1 (raw file):
registrationPromises
needs to be cleaned up for this web view when the web view is unmounted, right?
Cleanup of the map should live solely within the map. That's why cleanupPromiseChain
is private. Nothing outside of the map should be calling cleanupPromiseChain
. I don't know what timing issue you're talking about.
It seems
cleanupPromiseChain
doesn't work if new promises get added after it is run.
Can you be more specific about what you don't think is working? From my testing this works as I would expect (i.e., all registrations and unregistrations happen sequentially between different components with the same ID. There is no timing problem from what I've seen.
src/renderer/components/web-view.component.tsx
line 94 at r1 (raw file):
Previously, tjcouch-sil (TJ Couch) wrote…
Since this
useEffect
has stuff in its dependency array, the component will not necessarily be unmounted when its destructor runs. It will mean the current values are outdated and may mean the component is unmounted. Please rename this variable to avoid the confusion. I usually name variables similar to this oneisCurrent
or something like that. Though this variable means something slightly different - something like "is this request handler current or already registered and needs to be unregistered immediately". Maybe I'd recommendisThisRequestHandlerCurrentOrAlreadyRegisteredAndNeedsToBeUnregisteredImmediately
. Really channel our inner Java gurus.
I flipped the meaning and renamed to cleanupHasRun
.
src/renderer/components/web-view.component.tsx
line 150 at r1 (raw file):
I had trouble parsing through all the ... stuff here and got it only after a while of concentrated effort.
This describes my normal experience with most React-related code. I added more comments, but I'm not confident it will change the experience for others.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks for the adjustments and clarifications!
Reviewed 8 of 8 files at r2, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion
lib/platform-bible-utils/src/promise-chaining-map.ts
line 42 at r1 (raw file):
Previously, lyonsil (Matt Lyons) wrote…
Error handling is done within
cleanupPromiseChain()
on the next line. I think starting with a simple implementation that matches the current use case is enough. If something more complex is helpful/useful later, we can always add the functionality at that time and set defaults to match the current behavior.
Oh, I totally misread this. I didn't realize cleanupPromiseChain
is done here, so I didn't realize everything was automatically cleaned up. Sorry about that.
src/renderer/components/web-view.component.tsx
line 93 at r1 (raw file):
Previously, lyonsil (Matt Lyons) wrote…
registrationPromises
needs to be cleaned up for this web view when the web view is unmounted, right?Cleanup of the map should live solely within the map. That's why
cleanupPromiseChain
is private. Nothing outside of the map should be callingcleanupPromiseChain
. I don't know what timing issue you're talking about.It seems
cleanupPromiseChain
doesn't work if new promises get added after it is run.Can you be more specific about what you don't think is working? From my testing this works as I would expect (i.e., all registrations and unregistrations happen sequentially between different components with the same ID. There is no timing problem from what I've seen.
Resolved by clearing up my misunderstanding about cleanupPromiseChain
d376628
to
7700bf1
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @lyonsil)
src/renderer/components/web-view.component.tsx
line 94 at r1 (raw file):
Previously, lyonsil (Matt Lyons) wrote…
I flipped the meaning and renamed to
cleanupHasRun
.
Thanks! Sorry for the delay
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: complete! all files reviewed, all discussions resolved
The problem here is that when a window is moved within the docking framework, the webview component is unmounted and then remounted in a very short period of time. When the unmount happens, an RPC method is unregistered. When the mount happens again, the RPC method is registered again. However, when unmount and mount run asynchronously so closely to each other, the unregister call fires, and before it returns from the
main
process, the register call fails since the RPC client sees that the method is currently (still) registered. Then the unregister call frommain
returns, and the RPC client removes the method. That means when you moved the window, you lost the RPCpostMessage
endpoint. What gets confusing, then, is that when you move the window again, the unmount tries to unregister (which fails but otherwise does nothing), and the mount tries to register (which succeeds this time since there was nothing currently registered). So every other time you move the window, you have a functioning postMessage RPC method. The other half of the time, you don't have a functioning method.Fixing this involves sequencing the registration calls per webview to the RPC client. That way, the call to register doesn't occur until the call to unregister completes. No more race conditions.
This was made a little more complicated by the fact that when React unmounts a component, all values inside the component are destroyed. That means keeping track of the pending, chained promises cannot be done with the React component itself. It could be done, I think, via a Context object, but that's a little messier than I wanted. Since the component is created by a function inside of a module, I'm using a module-level map to store all the promise chains.
In case we run into this scenario somewhere else in the future, I created a class in our utils lib specifically for the special promise chain map. This might end up being the only place it gets used, but it seemed cleaner to separate out the functionality into its own class.
This change is