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
roblourens merged 2 commits intomicrosoft:masterfrom
digeff:fix_possible_race_condition
Jun 21, 2018
Merged

Fix possible race condition for new and changed scripts#336
roblourens merged 2 commits intomicrosoft:masterfrom
digeff:fix_possible_race_condition

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