Skip to content

Conversation

@jasonmalinowski
Copy link
Member

@jasonmalinowski jasonmalinowski commented Nov 15, 2021

File change notifications are async, and can come after we've removed the project from the workspace. As of this writing, this has happened approximately 58 million times since 16.0 according to fault telemetry.

Fixes https://devdiv.visualstudio.com/DevDiv/_workitems/edit/1433689

@jasonmalinowski jasonmalinowski requested a review from a team as a code owner November 15, 2021 23:48
@ghost ghost added the Area-IDE label Nov 15, 2021
@jasonmalinowski jasonmalinowski self-assigned this Nov 16, 2021
_eventSubscriptionTracker.Clear();
}

_documentFileChangeContext.Dispose();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should the dispose be in the lock, or are we guaranteed no race calls to RemoveFromWorkspace?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Once we're at this point, we know there's only one thread handling the disposal.

Private ReadOnly _lock As New Object
Private ReadOnly _watchedFiles As New List(Of WatchedEntity)
Private ReadOnly _watchedDirectories As New List(Of WatchedEntity)
Private _watchedFiles As ImmutableList(Of WatchedEntity) = ImmutableList(Of WatchedEntity).Empty
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not entirely sure why these were changed. Is it a perf concern? List actually seems better since we treat them as mutable under a lock.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To properly test this fix, I need to implement support in the mock to test the sequence of events of:

  1. Capture the list of watched entities.
  2. Unsubscribe from something.
  3. Raise the event from the captured list in 1.

Making this immutable means I can easily capture it for later.

@jasonmalinowski jasonmalinowski merged commit 4d98fa2 into dotnet:main Nov 16, 2021
@jasonmalinowski jasonmalinowski deleted the avoid-throwing-exceptions-during-unload branch November 16, 2021 20:54
@ghost ghost added this to the Next milestone Nov 16, 2021
@allisonchou allisonchou modified the milestones: Next, 17.1.P2 Nov 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants