-
Notifications
You must be signed in to change notification settings - Fork 694
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix threadpool load induced delayed responses to plugin requests #3141
Conversation
test/NuGet.Core.Tests/NuGet.Protocol.Tests/Plugins/Logging/AssemblyLogMessageTests.cs
Show resolved
Hide resolved
src/NuGet.Core/NuGet.Protocol/Plugins/InboundRequestProcessingHandler.cs
Outdated
Show resolved
Hide resolved
src/NuGet.Core/NuGet.Protocol/Plugins/InboundRequestProcessingHandler.cs
Outdated
Show resolved
Hide resolved
src/NuGet.Core/NuGet.Protocol/Plugins/InboundRequestProcessingHandler.cs
Show resolved
Hide resolved
src/NuGet.Core/NuGet.Protocol/Plugins/InboundRequestProcessingHandler.cs
Outdated
Show resolved
Hide resolved
test/NuGet.Core.Tests/NuGet.Protocol.Tests/Plugins/MessageDispatcherTests.cs
Outdated
Show resolved
Hide resolved
test/NuGet.Core.Tests/NuGet.Protocol.Tests/Plugins/MessageDispatcherTests.cs
Outdated
Show resolved
Hide resolved
test/NuGet.Core.Tests/NuGet.Protocol.Tests/Plugins/MessageDispatcherTests.cs
Show resolved
Hide resolved
test/NuGet.Core.Tests/NuGet.Protocol.Tests/Plugins/MessageDispatcherTests.cs
Outdated
Show resolved
Hide resolved
test/NuGet.Core.Tests/NuGet.Protocol.Tests/Plugins/MessageDispatcherTests.cs
Outdated
Show resolved
Hide resolved
…NuGet/NuGet.Client into dev-nkolev92-pluginExtraLogging
@@ -37,7 +38,7 @@ public sealed class InboundRequestContext : IDisposable | |||
IConnection connection, | |||
string requestId, | |||
CancellationToken cancellationToken) | |||
: this(connection, requestId, cancellationToken, PluginLogger.DefaultInstance) | |||
: this(connection, requestId, cancellationToken, new InboundRequestProcessingHandler(), PluginLogger.DefaultInstance) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was there any decision about extracting this to an Interface? Not sure if it should be something configurable where different Clients could want their own Request Handler. Maybe some want to restrict threads and don't care if it takes 5 minutes to begin processing a request in a CI, and others want to prioritize user experience?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All these types are internal.
So no one but NuGet can use a dedicated handler.
I did that on purpose because others do not need to make that decision, only the client does.
@@ -75,6 +79,11 @@ public sealed class InboundRequestContext : IDisposable | |||
throw new ArgumentNullException(nameof(logger)); | |||
} | |||
|
|||
if (inboundRequestProcessingHandler == null) | |||
{ | |||
throw new ArgumentNullException(nameof(inboundRequestProcessingHandler)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like it is important that this handler be specified. I'd expect the default constructor to be disabled so that nobody can create a Context without one:
private InboundRequestContext() { //disabled }
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's no default constructor.
The only public constructor creates a handler.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nevermind you're right
Bug
Fixes: NuGet/Home#8528
Regression: No
Fix
Details: The NuGet plugin communication happens through STDOUT/STDIN.
When NuGet receives something on standard output it is handled on a dedicated thread.
Once that happens we figure out what kind of message we have and delegate based on the type.
When we get a request to NuGet we put it on the threadpool
NuGet.Client/src/NuGet.Core/NuGet.Protocol/Plugins/InboundRequestContext.cs
Line 214 in 50fe55b
Because this comes from a dedicated thread and we flood the threadpool with requests, it's possible that it takes longer than 5 seconds to get to this request.
To mitigate, we will add a dedicated thread per NuGet side plugin that handles these requests.
The plugin side implementation will remain unchanged.
Resources for reading:
https://docs.microsoft.com/en-us/dotnet/api/system.threading.tasks.taskscheduler?view=netframework-4.8#the-default-task-scheduler-and-the-thread-pool
Note that this doesn't fix our issues with threadpool starvation (sometiems it's starvation, other times it's working as expected as far as the threadpool is concerned).
That's a separate task, but when plugins are involved, the starvation is more apparent.
Impl Details:
The InboundRequestProcessingHandler is the one responsible for ensuring the response task executes. It makes a decision whether to queue it on the threadpool or on the dedicated thread.
The dedicated thread implementation is a generic task executing implementation.
Note that only the NuGet side codepaths of the plugin code are affected.
Testing/Validation
Tests Added: Yes
Reason for not adding tests:
Validation: Manual, https://dev.azure.com/dnceng/internal/_build/results?buildId=441955 now has a 100% success rate as far as restore goes :)