fix: deal with delayed _nodeElectronHandle #6348
Conversation
|
|
pavelfeldman
left a comment
pavelfeldman
left a comment
There was a problem hiding this comment.
Thanks for the PR! I can repro the issue, so I'll be able to add a test for it.
src/server/electron/electron.ts
Outdated
|
|
||
| // Below is async. | ||
| const handle = await this._nodeElectronHandle!.evaluateHandle(({ BrowserWindow }, windowId) => BrowserWindow.fromId(windowId), windowId).catch(e => {}); | ||
| const handle = await (await this._nodeElectronHandle)!.evaluateHandle(({ BrowserWindow }, windowId) => BrowserWindow.fromId(windowId), windowId).catch(e => {}); |
There was a problem hiding this comment.
In case of mentioned race, this._nodeElectronHandle can still be undefined here.
src/server/electron/electron.ts
Outdated
| private _nodeSession: CRSession; | ||
| private _nodeExecutionContext: js.ExecutionContext | undefined; | ||
| _nodeElectronHandle: js.JSHandle<any> | undefined; | ||
| _nodeElectronHandle: Promise<js.JSHandle<any> | undefined>; |
There was a problem hiding this comment.
Promise<js.JSHandle<any>> | undefined;
There was a problem hiding this comment.
I'm not very sure whether '_nodeElectronHandle' will be reached before '_init' has finished or not, I just find '_onPage' and 'close' in server part.
It looks that '_init' will always finish before any '_onPage' when I have tested this PR in my application.
If we wanna ensure '_nodeElectronHandle' to be a Promise, maybe we could extract its initialization to a new method like 'initNodeHandle', and await launch process at top of it?
There was a problem hiding this comment.
Electron support uses two independent connections, one going to Node and another going to the browser. These connections race naturally and if you throttle the Node one, you can see how browser talks to us before node handle is initialized. You can add await new Promise(f => setTimeout(f, 1000)); before await this._nodeSession.send('Runtime.enable', {}).catch(e => {}); in the init to trigger it. You last push should resolve it! Let's see if it fixes my local test and if it does, we merge it. I'll follow up with the test and a small refactoring.
There was a problem hiding this comment.
@pavelfeldman Hope it works! I found some eslint issues in my commit, just fix and amend it to the only commit.
2 participants
Since
process.mainModule.require('electron')maybe be slow, we could possibly get undefined_nodeElectronHandle, so just treat it as a Promise.