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

make caching more robust; fixes #69534 #70073

Merged
merged 1 commit into from Mar 11, 2019
Merged

Conversation

weinand
Copy link
Contributor

@weinand weinand commented Mar 8, 2019

This PR fixes #69534.

When talking to the EH I was sending the debug session data only once in the first request that needed it. Then EH cached the data.

For subsequent request I'm sending only the session ID (and the EH looks up the session object from the cache). VS Code made the decision whether to send the full data or just the ID base on a "Set" data structure: after sending the data once, the ID is added to the Set.

After "workspaceService.resolveWorkspaceFolder()" had become asynchronous, the timing behavior has changed and now the EH cache does not have the data when a session ID arrives and needs to be looked up. This results in a "null" session and the error message on the console.

The fix is to introduce a robust (but still synchronous) caching protocol. For this the EH sends a new request "sessionCached" back to VS Code when it has cached the data. And VS Code only stops sending the full data if it has received the "sessionCached" request.

@weinand weinand assigned isidorn and unassigned isidorn Mar 8, 2019
@weinand weinand requested a review from isidorn March 8, 2019 15:49
@isidorn
Copy link
Contributor

isidorn commented Mar 8, 2019

Thanks for the explanation and the PR. I will review in more detail in two hours.

@isidorn
Copy link
Contributor

isidorn commented Mar 8, 2019

This PR looks good overall.

I have some questions:

  • Did we verify this fixes the issue?
  • Why did we originaly decide to sometimes send over the ID? Why don't we always send over the session (in that case this caching logic would not be needed)? In case we decide this makes sense we should do it in master, not for the fix.

However since we do not plan to merge this PR over the weekend we can discuss these things on monday morning in the office.

@weinand
Copy link
Contributor Author

weinand commented Mar 8, 2019

@isidorn The fix is already in Insiders and Dan has verified with his own build that it fixes the issue.

We decided to send only the ID when we started to always send the launch config with the session which makes the amount of data passed with every event and every request quite large.

@weinand weinand self-assigned this Mar 8, 2019
@weinand weinand added this to the February 2019 Recovery 2 milestone Mar 8, 2019
@isidorn
Copy link
Contributor

isidorn commented Mar 11, 2019

LGTM

@isidorn
Copy link
Contributor

isidorn commented Mar 11, 2019

@weinand just a ping / reminder to merge this in.

@weinand weinand merged commit e16e012 into release/1.32 Mar 11, 2019
@weinand weinand deleted the aweinand/fix_for_69534 branch March 11, 2019 14:14
@weinand weinand added the debug Debug viewlet, configurations, breakpoints, adapter issues label Mar 11, 2019
@egamma egamma removed this from the February 2019 Recovery 2 milestone Mar 14, 2019
@github-actions github-actions bot locked and limited conversation to collaborators Mar 27, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
debug Debug viewlet, configurations, breakpoints, adapter issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants