Make initialization of Connect synchronous#33444
Merged
Conversation
This is needed in order to register event listeners for deep linking ASAP on app start. Otherwise, the app won't be able to handle deep links on cold start.
avatus
approved these changes
Oct 13, 2023
Comment on lines
+49
to
+62
| /** | ||
| * createFileStorage reads and parses existing JSON structure from filePath or creates a new file | ||
| * under filePath with an empty object if the file is missing. | ||
| * | ||
| * createFileStorage itself uses blocking filesystem APIs but the functions of the returned | ||
| * FileStorage interface, such as write and replace, are async. | ||
| */ | ||
| // createFileStorage needs to be kept sync so that initialization of the app in main.ts can be sync. | ||
| // createFileStorage is called only during initialization, so blocking the main process during that | ||
| // time is acceptable. | ||
| // | ||
| // However, functions such as write or replace returned by createFileStorage need to be async as | ||
| // those are called after initialization. | ||
| export function createFileStorage(opts: { |
Contributor
There was a problem hiding this comment.
The two comment styles here struck me as odd but now I understand that the comments under the jsdoc style comments are actually omitted in the documentation. neat trick
Member
Author
There was a problem hiding this comment.
Yeah, I didn't want the comment about the function internals pollute the JSDoc comment. I could've moved the // comment to be inside the function, but since it's about createFileStorage in general, I decided to keep it there.
public-teleport-github-review-bot
Bot
removed the request for review
from gzdunek
gzdunek
approved these changes
Oct 16, 2023
|
@ravicious See the table below for backport results.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
In order to support deep links (#32188), we need to add listeners for certain events to make the app respond to opening it from a custom protocol link in the Web UI.
While playing with it, I noticed that nothing would happen if I clicked the link while the app wasn't running, the app would launch but it didn't show the dialog box I wired up in the event listener.
It turns out it's because
initializeAppwhere I added the listener, and where we keep most of our otherappandprocesslisteners, is async. This means it doesn't get processed immediately on app launch, at best it happens during the next tick. This is too late for registering theopen-urllistener and the event is simply dropped.This PR replaces the async actions during initialization with their sync equivalents, making
initializeAppsync. The only async actions were file system operations related tocreateFileStorage– this is a function we use to store and read certain JSON blobs from disk, such as app config and app state that is persisted between app launches.I packaged the app on all three platforms and verified that it launches without any problems.
Blocking the main process 😈
Generally, Electron discourages blocking the main process. If the main process is blocked, the UI will freeze until the main process is unblocked.
However, during initialization there's no UI. The main app window is not even shown until we call
windowsManager.createWindow()inmain.ts. Currently,windowsManager.createWindow()gets called only after the calls tocreateFileStoragefinish. This means two things:createFileStorageare blocked on filesystem access, even today this means that the user will not see the window until those operations finish.This means that this change doesn't introduce any adverse effects for the user, unless there's something I'm not aware of related to what Electron does during app start.
An alternative
An alternative to blocking the main process would be to segregate what happens during startup. We'd set up the listeners in sync code and the rest of the initialization process, such as calling
createFileStorage, would happen in async code.I did craft a quick proof of concept. The problem with this approach is that the
open-urlhandler would likely need to know when the frontend app is ready in some way. Currently, all IPC communication is set up through ourMainProcessclass which depends on a couple of file storages initialized throughcreateFileStorage.So we'd either have to solve this problem or add some IPC handlers in another object which doesn't depend on file storage. This is not impossible but certainly adds some complexity.
Blocking the main process 😌
I did some quick tests where I tested the current app initialization, the segregated approach and the sync approach. With
performance.now(), I measured the type from initial load ofmain.ts(taking a measurement right after imports) and then made another measurement onceapp.whenReadyhas resolved.On both my MBP and on Windows VM, the sync version performed better than others.
My MBP:
Windows VM: