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

Add missing return to FileStorage.write #41958

Merged
merged 1 commit into from
May 23, 2024
Merged

Conversation

ravicious
Copy link
Member

Ever since we switched to Vite and started using electron-vite to automatically restart the Electron app on changes in dev mode, I sometimes noticed my app_state.json getting corrupted and the app starting in a pristine state, showing modals as if it was launched for the first time.

I attributed this to app_state.json getting corrupted. I looked for a relevant error in my logs and sure enough there it was:

[23-05-24 10:35:16] [FileStorage] error: Cannot read /Users/rav/Library/Application Support/Electron/app_state.json file SyntaxError: Unexpected end of JSON input
    at JSON.parse (<anonymous>)
    at loadStateSync (/Users/rav/src/teleport/web/packages/teleterm/build/app/main/index.js:909:15)
    at createFileStorage (/Users/rav/src/teleport/web/packages/teleterm/build/app/main/index.js:857:13)
    at createFileStorages (/Users/rav/src/teleport/web/packages/teleterm/build/app/main/index.js:75420:26)
    at initializeApp (/Users/rav/src/teleport/web/packages/teleterm/build/app/main/index.js:75246:7)

I suspected that it might be due to electron-vite sending a sigkill to the Electron app and the app getting killed while it's writing to the file. I asked Grzegorz if he ever ran into that, and he confirmed that he did run into that too.

Grzegorz said, "I wonder if changing appStateFileStorage.write() to a sync write instead of async-based one would help here", and I said, "I don't think so, it's already awaited for…"

app.on('will-quit', async event => {
event.preventDefault();
const disposeMainProcess = async () => {
try {
await mainProcess.dispose();
} catch (e) {
logger.error('Failed to gracefully dispose of main process', e);
}
};
globalShortcut.unregisterAll();
await Promise.all([appStateFileStorage.write(), disposeMainProcess()]); // none of them can throw
app.exit();
});

Well, turns out it wasn't, because we failed to return a promise from FileStorage.write! This is one of the things that no-floating-promises would have caught (#41945).

@public-teleport-github-review-bot public-teleport-github-review-bot bot removed the request for review from rudream May 23, 2024 16:40
@ravicious ravicious added this pull request to the merge queue May 23, 2024
Merged via the queue into master with commit 2617878 May 23, 2024
39 of 40 checks passed
@ravicious ravicious deleted the r7s/wait-for-write branch May 23, 2024 17:05
@public-teleport-github-review-bot

@ravicious See the table below for backport results.

Branch Result
branch/v13 Create PR
branch/v14 Create PR
branch/v15 Create PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants