Skip to content

EnC manager replacement#34027

Closed
tmat wants to merge 1 commit intodotnet:master-vs-depsfrom
tmat:EncManagerUpdate
Closed

EnC manager replacement#34027
tmat wants to merge 1 commit intodotnet:master-vs-depsfrom
tmat:EncManagerUpdate

Conversation

@tmat
Copy link
Member

@tmat tmat commented Mar 12, 2019

Replaces legacy EnC manager implementation with a new one.

The previous implementation of the EnC manager in the debugger communicated with language services via project system coupled interfaces (IVsENCRebuildableProjectCfg*). This had many limitations, including necessity to perform IO synchronously on UI thread, lack of support for multi-targeting, necessity to make buffers readonly while application is running and for projects whose modules were not loaded to the debugee process etc.

The new architecture completely removes the dependency on Project System. The debugger communicates with language services via a MEF component. Language services export

  1. IDebugStateChangeListener
    The implementation of this interface receives callbacks from the debugger when the debug state changes:
        void EnterBreakState(BreakStateKind kind);
        void ExitBreakState();
        void StartDebugging();
        void StopDebugging();
  1. IEditAndContinueManagedModuleUpdateProvider

The implementation responds to the debugger's requests for updates in managed modules.
The debugger sends these requests when changes made in source files need to be applied due to a user action (e.g. "continue" in break mode, change current IP while in break mode, etc.).

Avoiding read-only buffers

Previously we made buffers read only in scenarios where we couldn't apply the change at the end of the current edit session ("continue"). This could be for multiple reasons:

  1. Changing source files while the application is running
    Changes can only be applied when the debuggee is stopped.

We let the user made changes while the debuggee is running. We report a warning for these changes notifying the user that the changes are not being applied while the debugee is running.

When the debugger stops on a breakpoint in a modified source, the source won't match the one that's being executed so based on the settings the debugger may display a dialog saying so. We will follow up to improve that experience, but not going to block the change on it.

  1. Changing source files of projects during edit session whose corresponding module is not loaded to the debugee
    Since the modules are not loaded we can't apply the delta at the end of the edit session (there is no module in the debugee process to apply the delta to).

We calculate the EnC deltas for all projects changed during edit session regardless of whether their modules are loaded or not. At the end of the session we apply deltas only to modules that are loaded, but we remember all the deltas we calculated for next opportunity to apply changes. Such opportunity occurs when a module is loaded while the debuggee is running. At that point the debuggee is stopped while the even is processed and we can apply all deltas that were made to the project that corresponds to the module. Note that this also handles the case where multiple instances of the same module are loaded to the debuggee (e.g. to different AppDomains or AssemblyLoadContexts). The previous design didn't account for this scenario resulting in bugs like #34253.

  1. Other reasons that disallow EnC (e.g. being stopped at exception, the debuggee process not supporting EnC, etc.).

We do not block the user from making changes in the source files. Instead we report regular errors like we do for other Rude Edits. We use new APIs added to Concord to support querying for availability of EnC for specific module.

Fixes #10203.
Fixes #11656.
Fixes #18917.
Fixes #21170.
Fixes #27373.
Fixes #27735.
Fixes #29223.
Fixes #34253.
Fixes DevDiv 750649
Fixes DevDiv 551604

See also #18350.

@CyrusNajmabadi
Copy link
Contributor

I'm interested in reviewing this once it is out of draft. Tnx!

@tmat
Copy link
Member Author

tmat commented Mar 12, 2019

@CyrusNajmabadi Cool. It's not everyday that someone actually wants to review EnC PR :)

@tmat tmat force-pushed the EncManagerUpdate branch 2 times, most recently from 41e7711 to f1e1294 Compare March 22, 2019 23:57
@tmat
Copy link
Member Author

tmat commented Mar 23, 2019

@CyrusNajmabadi Still waiting for debugger to implement some APIs, need to write tests and do a bit more refactoring but the basics are in place. Feel free to take a look.

Also need to write some design doc.

@tmat tmat force-pushed the EncManagerUpdate branch 3 times, most recently from d91ec9c to a7c8370 Compare March 28, 2019 17:40
@tmat tmat changed the title Enc manager update EnC manager replacement Mar 28, 2019
@tmat tmat force-pushed the EncManagerUpdate branch 2 times, most recently from 58a417e to fb64cc7 Compare April 12, 2019 23:41
@tmat tmat added this to the 16.2 milestone May 8, 2019
@tmat tmat force-pushed the EncManagerUpdate branch from fb64cc7 to 71b50cb Compare May 10, 2019 02:19
@tmat tmat marked this pull request as ready for review May 17, 2019 20:40
@tmat tmat requested review from a team as code owners May 17, 2019 20:40
@sharwell sharwell dismissed their stale review May 24, 2019 19:33

All feedback was responded to

@tmat tmat force-pushed the EncManagerUpdate branch 2 times, most recently from 92acdbf to 401f179 Compare May 24, 2019 23:41
@tmat tmat force-pushed the EncManagerUpdate branch from bdb93a6 to 5041867 Compare May 28, 2019 19:29
@tmat
Copy link
Member Author

tmat commented May 28, 2019

@ivanbasov Any more feedback?

Copy link
Contributor

@ivanbasov ivanbasov left a comment

Choose a reason for hiding this comment

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

:shipit:

@tmat tmat force-pushed the EncManagerUpdate branch from 5041867 to a3fe1e6 Compare May 29, 2019 00:08
@tmat tmat changed the base branch from master-vs-deps to release/dev16.2-preview2-vs-deps May 29, 2019 00:08
@tmat tmat changed the base branch from release/dev16.2-preview2-vs-deps to master-vs-deps May 29, 2019 00:09
@tmat
Copy link
Member Author

tmat commented May 30, 2019

Superseded by #36010

@tmat tmat closed this May 30, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants