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

Fix possible race condition for new and changed scripts #336

Merged
merged 2 commits into from
Jun 21, 2018

Conversation

digeff
Copy link
Contributor

@digeff digeff commented Jun 20, 2018

We saw on one of our automated tests that we sent a "changed" event for a script before the "new" event. This fixes one potential cause.

@roblourens
Copy link
Member

I don't understand what this is fixing, nothing can happen between the top of that method and the original place where scriptToSource was called, right?

@digeff
Copy link
Contributor Author

digeff commented Jun 21, 2018

If we get the same script twice, and we put the reason 'new' first, and 'changed' later, and then we call this.scriptToSource() maybe the 'new' event will get blocked for longer than the 'changed' event', and we'll end up sending the 'changed' event before the 'new' event.

We've seen that behavior happen in our tests, and VS fails because of the unexpected order. I don't know for sure if this is the cause, but it's one of the potential places that could be causing that.

@roblourens
Copy link
Member

Blocked by what? In javascript, the thread won't be interrupted in sync code. There are no awaits between lines 863 and 882 so I don't see how that would happen. Does this fix the problem in your tests?

@roblourens roblourens merged commit f4caf32 into microsoft:master Jun 21, 2018
@roblourens roblourens added this to the June 2018 milestone Jun 21, 2018
@digeff digeff deleted the fix_possible_race_condition branch June 21, 2018 17:29
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants