This repository was archived by the owner on Oct 2, 2021. It is now read-only.

Block while parsing scripts to prevent sending 'removes' before 'adds'#274

Merged

roblourens merged 7 commits intomicrosoft:masterfrom

digeff:block_while_parsing_scripts

Feb 2, 2018

Contributor

digeff commented Jan 29, 2018

We were running into an issue where we send 'remove' myscript.js before we sent the 'add' myscript.js when we went into a page, and quickly redirected into another page.

That happened because we have some async code in how we process onScriptParsed, so that processing can be on-flight, and onExecutionContextsCleared very quickly sends the 'remove' events, and we weren't waiting for the onScriptParsed events to be completed.
This fixes solve part of the issue.

I think there is another unrelated reason that I'm investigating after this.

vscodebot bot assigned roblourens

Member

roblourens commented Jan 29, 2018

I don't think this will fix your problem. await won't keep the code in onExecutionContextsCleared from running. Your change will keep the rest of the code in onScriptParsed from running before all the scriptParsed notifications are sent, but won't stop other event handlers.

You probably want a promise that you use as an async queue, like

private _scriptParsedQueue: Promise<any>;

...

onScriptParsed() {
  this._scriptParsedQueue = this._scriptParsedQueue.then(() => send load events)
}

onExecutionContextsCleared() {
  this._scriptParsedQueue.then(() => send delete events)
}

Member

roblourens commented Jan 31, 2018

The problem is that we are getting a breakpointResolved event for the new script before you have processed the scriptLoaded event, so the breakpointResolved handler stops at this line: https://github.com/Microsoft/vscode-chrome-debug-core/blob/master/src/chrome/chromeDebugAdapter.ts#L930

Two options:

Only delay the sending of the events to the client, not the whole onScriptParsed handler
Have the breakpointResolved handler also wait on the scriptParsedQueue

I think the first one is better for perf, and would hopefully prevent other issues with things that depend on the _scriptsById map.

Member

roblourens commented Jan 31, 2018

Could you add a test to the PR as well, that adds some scripts, refreshes the page, and ensures that the script load/unload events are sent in the proper order? You can take some inspiration from this failing test on how to do that.

Diego Geffner added 5 commits

January 31, 2018 16:48


          Block while parsing scripts to prevent sending 'removes' before 'adds'


          Applied feedback

e6e7312


          Add unit test

6a835cf


          Made all cases work

6f63b31


          Changed to a typescript version without the bug

4a5ce3a

Contributor Author

digeff commented Feb 1, 2018

Ran into a bug with the newest typescript: microsoft/TypeScript#21491
changing to an older version

digeff force-pushed the block_while_parsing_scripts branch from b2b9611 to 4a5ce3a Compare

February 1, 2018 01:11

roblourens suggested changes

View reviewed changes

src/chrome/chromeDebugAdapter.ts Outdated

                           this._initialSourceMapsP = null;
-                          initialSourceMapsP.then(() => {
+                          await initialSourceMapsP.then(async () => {

Member

roblourens Feb 2, 2018

The mixing of async/await and promises is a little confusing here (await ... .then()

Contributor Author

digeff Feb 2, 2018

Done

src/chrome/chromeDebugAdapter.ts Outdated

    
                          await initialSourceMapsP.then(async () => {

                              this._session.sendEvent(new InitializedEvent());

                              this._earlyScripts.forEach(script => this.sendLoadedSourceEvent(script));

                              await Promise.all(this._earlyScripts.map(async script => await this.sendLoadedSourceEvent(script)));

Member

roblourens Feb 2, 2018

The async/await inside map is redundant, just return the promise from this.sendLoadedSourceEvent

Contributor Author

digeff Feb 2, 2018

Done

src/chrome/chromeDebugAdapter.ts

                       }
                   }
+                  public doAfterProcessingSourceEvents(action: () => void): Promise<void> {

Member

roblourens Feb 2, 2018

action returns a Promise

Contributor Author

digeff Feb 2, 2018

The use inside onExecutionContextsCleared() doesn't return a promise

src/chrome/chromeDebugAdapter.ts Outdated

-                      Promise.all(asyncOperations).then(() =>
-                          this.clearTargetContext());
+                          return Promise.all(asyncOperations).then(() =>

Member

roblourens Feb 2, 2018 •

edited

Loading

This is confusing, asyncOperations will not be populated by this point because it's populated by an async function that we don't wait on. I'm not sure what's supposed to happen.

Contributor Author

digeff Feb 2, 2018

Nice catch... I missed this...

src/chrome/chromeDebugAdapter.ts Outdated

-                              this._session.sendEvent(scriptEvent)));
-                      });
+                  private onExecutionContextsCleared(): Promise<void> {
+                          const asyncOperations = [];

Member

roblourens Feb 2, 2018

I think this method picked up an extra level of indentation

Contributor Author

digeff Feb 2, 2018

Done


          Applied feedback

e9b9a24

roblourens reviewed

View reviewed changes

src/chrome/chromeDebugAdapter.ts Outdated

-                      Promise.all(asyncOperations).then(() =>
-                          this.clearTargetContext());
+                      return this.doAfterProcessingSourceEvents(async () => { // Execute after sending all the 'removed' events

Member

roblourens Feb 2, 2018

This is a confusing flow now. The asyncOperations array isn't needed if doAfterProcessingSourceEvents is forcing this to happen after sending the script removed events are sent.

Also, you could iterate the scripts inside the doAfterProcessingSourceEvent. That will be faster because the script processing doesn't have to happen one by one. You can use the await Promise.all(scriptById.map(async () => ... pattern again.

Contributor Author

digeff Feb 2, 2018

I rewrote this to use async/wait. What do you think?


          Rewrote method to use async/wait

9b62e44

roblourens approved these changes

View reviewed changes

Member

roblourens left a comment

I think this is right. Async code is hard :) Thanks!

roblourens merged commit c07dc3f into microsoft:master

digeff deleted the block_while_parsing_scripts branch

February 2, 2018 21:26

roblourens added this to the February 2018 milestone

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet