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

Fix #213885, As much as possible flush the storage in beforeunload callback #213896

Closed
wants to merge 11 commits into from

Conversation

yiliang114
Copy link
Contributor

Fix #213885

…ge service flush when triggered by beforeunload
@bpasero
Copy link
Member

bpasero commented May 30, 2024

From what I know working in this area, there is no way to make beforeUnload long running. Browsers do not guarantee that you can execute any async code during this event. So I am not sure this fixes anything?

@bpasero bpasero self-assigned this May 30, 2024
@bpasero bpasero added the info-needed Issue requires more information from poster label May 30, 2024
@yiliang114
Copy link
Contributor Author

yiliang114 commented May 30, 2024

From what I know working in this area, there is no way to make beforeUnload long running. Browsers do not guarantee that you can execute any async code during this event. So I am not sure this fixes anything?据我所知,在这个领域工作,没有办法让 beforeUnload 长时间运行。浏览器不保证您可以在此事件期间执行任何异步代码。所以我不知道这个修复什么?

My original intention is to ensure that the storgeService.flush is executed before the page is refreshed to ensure that the state can be written normally. Before that, although the callback function for the beforeUnload event is synchronous, the storgeService.flush function is a promise, and from the phenomenon, the state is not written normally before the page is refreshed.

I would like to paste the comparison of the effect before and after I modified these lines of code to let you know my purpose:

before:
reload---1

after:
reload---2

However, you mentioned that the browser does not guarantee that the asynchronous callback function can be executed. Indeed, I did not notice this problem before. I want to think again.

@yiliang114
Copy link
Contributor Author

hi @bpasero I have some new progress:

  1. originally I tried to write the callback function as asynchronous. although I have tested many times and found that the asynchronous function can be executed before overloading the whole page, I have consulted many documents and most people say that the browser only guarantees that the synchronous function can be executed, so I think it is not a good solution.
  2. Now my idea is to also use a vote identification bit to evoke the confirmation box as much as possible when the refresh cache task is not completed.

https://github.com/yiliang114/vscode/blob/7a3345a553dd02f06e2acdc3e56173a36c1b65da/src/vs/workbench/services/lifecycle/browser/lifecycleService.ts#L119

The final effect is something like this:
reload---3

@yiliang114
Copy link
Contributor Author

@bpasero
Copy link
Member

bpasero commented May 30, 2024

I doubt it will work, there is no async way to prevent the unload. I am not sure what this PR is trying to solve.

@yiliang114 yiliang114 changed the title Fix #213885, make doShutdown asynchronous and wait for storage service flush when triggered by beforeunload Fix #213885, As much as possible in the beforeunload, flush the storage May 31, 2024
@yiliang114
Copy link
Contributor Author

I doubt it will work, there is no async way to prevent the unload. I am not sure what this PR is trying to solve.我怀疑它会工作,没有异步的方式来防止卸载。我不知道这是什么公关试图解决。

I doubt it will work, there is no async way to prevent the unload. I am not sure what this PR is trying to solve.我怀疑它会工作,没有异步的方式来防止卸载。我不知道这是什么公关试图解决。

Yes, In fact, it is no longer an asynchronous function blocking the browser. I just changed the title.

The current strategy is that if you need to flush storage before the page unload, you will call up the confirmation box as much as possible. Although there is no guarantee that the flush task will be completed, there is a chance, just like your previous code.

@yiliang114 yiliang114 changed the title Fix #213885, As much as possible in the beforeunload, flush the storage Fix #213885, As much as possible flush the storage in beforeunload callback May 31, 2024
@bpasero
Copy link
Member

bpasero commented May 31, 2024

We explicitly do not want to block the unload only for that reason because it is very annoying and there is not even a way to tell the user what is going on. Instead, we periodically flush storage time based and also when focus changes.

Sorry, but I think there is not a lot we can do here unless browser vendors give in and allow for more control what happens on unload.

@yiliang114
Copy link
Contributor Author

We explicitly do not want to block the unload only for that reason because it is very annoying and there is not even a way to tell the user what is going on. Instead, we periodically flush storage time based and also when focus changes.我们明确地不想仅仅因为这个原因而阻止卸载,因为它非常烦人,甚至没有办法告诉用户发生了什么。相反,我们会定期刷新基于存储时间以及焦点更改时的存储时间。

Sorry, but I think there is not a lot we can do here unless browser vendors give in and allow for more control what happens on unload.对不起,但我认为没有很多,我们可以在这里做,除非浏览器供应商让步,并允许更多的控制卸载会发生什么。

Thank you very much! I see what you mean.

Then I would also like to ask other question, at the time when I operate vscode (for example close the panel or open some files) I refresh the page directly, some states may not be saved. Do you have any good suggestions for solving this problem?

@bpasero
Copy link
Member

bpasero commented May 31, 2024

I actually do not, but I am happy for suggestions that do not force a modal dialog on people 🙏

@bpasero
Copy link
Member

bpasero commented Jun 4, 2024

This should be fixed now.

@bpasero bpasero closed this Jun 4, 2024
@vs-code-engineering vs-code-engineering bot locked and limited conversation to collaborators Jul 19, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
info-needed Issue requires more information from poster
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Panel visibility is not persisted/restored on vscode.dev after restart
6 participants