Refactor request invocation to be extensible. #641
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- The initial motivation for this change spawned from an investigation in the Razor language server where transitioning from Parallel -> Serial tasks would wait on Parallel work to finish. In scenarios when that parallel work took a long time this could result in significant editor delays in completion where you'd have Parallel (long) -> Serial -> Parallel (completion, short). I played around with changing the System.Reactive bits to have an option to not "wait" for the parallel stacks but System.Reatcive as a library wasn't truly built to handle that type of "change your mind after-the-fact" flow. - Prior to this change the routing & scheduling aspects of the JsonRpc stack are bound to our ProcessScheduler & InputHandler.RouteRequest & InputHandler.RouteNotification endspoints. This change allows that entire stack to be extensible so consumers can plug & play. - Added a `RequestInvoker` type which represents the core logic of how the framework invokes a handler for a request. This encapsulates the control flow for invoking, scheduling and handling fallout from invoking a handler. - Added a `RequestInvokerOptions` type to represent what sort of settings should be applied for the request invoker paradigm. - Expanded `InputHandler` & `Connection` to have two new constructors that take in a request invoker and obsoleted the old ones. Updated tests to account for this. - Registered the default request invoker type (the one that uses System.Reactive) if a request invoker was not already registered.
- For consumers who are creating their own `RequestInvoker` they need to manually construct many of our response types. Therefore, the constructors need to also be puclic.
Codecov Report
@@ Coverage Diff @@
## master #641 +/- ##
==========================================
+ Coverage 69.41% 69.43% +0.01%
==========================================
Files 257 260 +3
Lines 12580 12624 +44
Branches 848 853 +5
==========================================
+ Hits 8732 8765 +33
- Misses 3578 3584 +6
- Partials 270 275 +5
Continue to review full report at Codecov.
|
NTaylorMullen
commented
Aug 23, 2021
| Observable.Create<ErrorResponse>( | ||
| observer => { | ||
| // ITS A RACE! | ||
| var sub = Observable.Amb( |
There was a problem hiding this comment.
All this is verbatim from the original RouteRequest method in InputHandler.
NTaylorMullen
added a commit
to dotnet/razor
that referenced
this pull request
Aug 24, 2021
- With the latest OmniSharp upgrade there were a few high profile changes:
- Most DTO's are records now which makes them immutable. You'll see throughout the codebase that we now have to use `with {...}` to translate one type to another.
- `ServerCapabilities` are expandable by default. This means we no longer need our `ExtendableServerCapabilities` type.
- Getting client and server capabilities in each of our endpoints are now a single method call. This resulted in lots of deleted code.
- O# upgraded its LSP version to 3.17 which means semantic tokens are no longer proposed. This resulted in a lot of warnings/obsolete bits getting removed. We now also have code action resolution as part of this upgrade so we could remove our old code action resolution endpoint (it's in VSCode now too).
- This changeset is in preparation for another O# release where we'll replace the [O# request invoker](OmniSharp/csharp-language-server-protocol#641) to get some pretty massive perf wins!
Fixes dotnet/aspnetcore#35622
NTaylorMullen
added a commit
to dotnet/razor
that referenced
this pull request
Aug 24, 2021
- With the latest OmniSharp upgrade there were a few high profile changes:
- Most DTO's are records now which makes them immutable. You'll see throughout the codebase that we now have to use `with {...}` to translate one type to another.
- `ServerCapabilities` are expandable by default. This means we no longer need our `ExtendableServerCapabilities` type.
- Getting client and server capabilities in each of our endpoints are now a single method call. This resulted in lots of deleted code.
- O# upgraded its LSP version to 3.17 which means semantic tokens are no longer proposed. This resulted in a lot of warnings/obsolete bits getting removed. We now also have code action resolution as part of this upgrade so we could remove our old code action resolution endpoint (it's in VSCode now too).
- The way the O# serializer gets construed now is different and extendable. Because of this we now have a primary method to add all of our converters to an O# serializer.
- O# embraced the optional vs. required text document identifiers. This makes it super clear whenever we're expected to provided a document version or not. Probably one of my favorite changes in the upgrade.
- A new dependency of `System.Threading.Channels` was introduced so we had to make sure that was included in our VS scenarios.
- This changeset is in preparation for another O# release where we'll replace the [O# request invoker](OmniSharp/csharp-language-server-protocol#641) to get some pretty massive perf wins!
Fixes dotnet/aspnetcore#35622
david-driscoll
requested changes
Aug 24, 2021
|
|
||
| if (!container.IsRegistered<RequestInvoker>()) | ||
| { | ||
| container.Register<RequestInvoker, DefaultRequestInvoker>(Reuse.Singleton); |
There was a problem hiding this comment.
This is missing .Type<IScheduler>(serviceKey: nameof(options.InputScheduler))
There was a problem hiding this comment.
ooo, out of curiosity why not have that be registered by default?
|
🆙 📅 |
ghost
pushed a commit
to dotnet/razor
that referenced
this pull request
Aug 24, 2021
* Upgrade OmniSharp to 0.19.4.
- With the latest OmniSharp upgrade there were a few high profile changes:
- Most DTO's are records now which makes them immutable. You'll see throughout the codebase that we now have to use `with {...}` to translate one type to another.
- `ServerCapabilities` are expandable by default. This means we no longer need our `ExtendableServerCapabilities` type.
- Getting client and server capabilities in each of our endpoints are now a single method call. This resulted in lots of deleted code.
- O# upgraded its LSP version to 3.17 which means semantic tokens are no longer proposed. This resulted in a lot of warnings/obsolete bits getting removed. We now also have code action resolution as part of this upgrade so we could remove our old code action resolution endpoint (it's in VSCode now too).
- The way the O# serializer gets construed now is different and extendable. Because of this we now have a primary method to add all of our converters to an O# serializer.
- O# embraced the optional vs. required text document identifiers. This makes it super clear whenever we're expected to provided a document version or not. Probably one of my favorite changes in the upgrade.
- A new dependency of `System.Threading.Channels` was introduced so we had to make sure that was included in our VS scenarios.
- This changeset is in preparation for another O# release where we'll replace the [O# request invoker](OmniSharp/csharp-language-server-protocol#641) to get some pretty massive perf wins!
Fixes dotnet/aspnetcore#35622
* Addressed code review comments
- Set => Init
- Removal of semantic token legend endpoint
- Some refactoring in our serializer extensions
- Test fixups
david-driscoll
approved these changes
Aug 27, 2021
|
Dopeness! Thank you @david-driscoll! Would it be possible to publish another version of the langauge server with this? |
|
Very cool work here! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
RequestInvokertype which represents the core logic of how the framework invokes a handler for a request. This encapsulates the control flow for invoking, scheduling and handling fallout from invoking a handler.RequestInvokerOptionstype to represent what sort of settings should be applied for the request invoker paradigm.InputHandler&Connectionto have two new constructors that take in a request invoker and obsoleted the old ones. Updated tests to account for this.Make existing request response types fully public
RequestInvokerthey need to manually construct many of our response types. Therefore, the constructors need to also be puclic.