Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds an HTTP endpoint to allow runtime configuration of the Durable Task extension for gRPC protocol. This addresses scenarios where function metadata is unavailable for protocol detection, particularly in legacy function.json-based programming models without worker indexing.
Changes:
- Adds a new
/management/{configureGrpc}HTTP endpoint that clears cached task hub workers and configures gRPC protocol - Refactors
GetClientmethod intoCreateClientAttributeFromRequestandGetClientfor better separation of concerns - Adds
ClearCachedTaskHubWorkermethod toDurableTaskExtensionfor invalidating cached workers
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 8 comments.
| File | Description |
|---|---|
| src/WebJobs.Extensions.DurableTask/HttpApiHandler.cs | Adds new management endpoint route, handler method, and refactors client attribute creation logic |
| src/WebJobs.Extensions.DurableTask/DurableTaskExtension.cs | Adds method to clear cached task hub worker before reconfiguring protocol |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| this.taskHubWorker.Dispose(); | ||
| this.taskHubWorker = null; |
There was a problem hiding this comment.
The ClearCachedTaskHubWorker method will throw a NullReferenceException if taskHubWorker is null when Dispose is called. Consider adding a null check before disposing, or using the null-conditional operator.
| this.taskHubWorker.Dispose(); | |
| this.taskHubWorker = null; | |
| if (this.taskHubWorker != null) | |
| { | |
| this.taskHubWorker.Dispose(); | |
| this.taskHubWorker = null; | |
| } |
| this.taskHubWorker.Dispose(); | ||
| this.taskHubWorker = null; |
There was a problem hiding this comment.
The ClearCachedTaskHubWorker method accesses and modifies the taskHubWorker field without synchronization, while other methods like ConfigureForGrpcProtocol use protocolLockObject for thread safety. This creates a potential race condition where multiple threads could simultaneously access or modify taskHubWorker. Consider adding lock protection around this operation using the same protocolLockObject to maintain consistency with the existing synchronization pattern.
| this.taskHubWorker.Dispose(); | |
| this.taskHubWorker = null; | |
| lock (this.protocolLockObject) | |
| { | |
| if (this.taskHubWorker != null) | |
| { | |
| this.taskHubWorker.Dispose(); | |
| this.taskHubWorker = null; | |
| } | |
| } |
| private async Task<HttpResponseMessage> HandleConfigureGrpcRequest(HttpRequestMessage request) | ||
| { | ||
| this.config.ClearCachedTaskHubWorker(); | ||
| this.config.ConfigureForGrpcProtocol(); | ||
|
|
||
| var attribute = this.CreateClientAttributeFromRequest(request); | ||
| IDurableOrchestrationClient client = this.GetClient(request, attribute); | ||
| var bindings = new BindingHelper(this.config); | ||
| string clientString = bindings.DurableOrchestrationClientToString(client, attribute); | ||
|
|
||
| return request.CreateResponse(HttpStatusCode.OK, clientString); | ||
| } |
There was a problem hiding this comment.
The HandleConfigureGrpcRequest endpoint does not validate the HTTP method being used. Looking at other endpoints in this file (lines 315, 344, 398, 431), they typically check for specific HTTP methods and return HttpStatusCode.NotFound for unsupported methods. Consider adding a check to ensure this endpoint only responds to the appropriate HTTP method (likely POST given it modifies state).
| private async Task<HttpResponseMessage> HandleConfigureGrpcRequest(HttpRequestMessage request) | ||
| { | ||
| this.config.ClearCachedTaskHubWorker(); | ||
| this.config.ConfigureForGrpcProtocol(); | ||
|
|
||
| var attribute = this.CreateClientAttributeFromRequest(request); | ||
| IDurableOrchestrationClient client = this.GetClient(request, attribute); | ||
| var bindings = new BindingHelper(this.config); | ||
| string clientString = bindings.DurableOrchestrationClientToString(client, attribute); | ||
|
|
||
| return request.CreateResponse(HttpStatusCode.OK, clientString); | ||
| } |
There was a problem hiding this comment.
The new HandleConfigureGrpcRequest endpoint lacks test coverage. The HttpApiHandlerTests.cs file contains comprehensive tests for other HTTP endpoints in this class. Consider adding tests to verify: the endpoint correctly clears the cached task hub worker, configures gRPC protocol, returns the expected HTTP status code, and returns the correct client string format.
| private async Task<HttpResponseMessage> HandleConfigureGrpcRequest(HttpRequestMessage request) | ||
| { | ||
| this.config.ClearCachedTaskHubWorker(); | ||
| this.config.ConfigureForGrpcProtocol(); | ||
|
|
||
| var attribute = this.CreateClientAttributeFromRequest(request); | ||
| IDurableOrchestrationClient client = this.GetClient(request, attribute); | ||
| var bindings = new BindingHelper(this.config); | ||
| string clientString = bindings.DurableOrchestrationClientToString(client, attribute); | ||
|
|
||
| return request.CreateResponse(HttpStatusCode.OK, clientString); | ||
| } |
There was a problem hiding this comment.
This method is marked as async but contains no await operations. The async keyword is unnecessary here and creates a state machine overhead for no benefit. Consider removing the async keyword and returning Task.FromResult or directly wrapping the synchronous result, which would improve performance.
| private async Task<HttpResponseMessage> HandleConfigureGrpcRequest(HttpRequestMessage request) | ||
| { | ||
| this.config.ClearCachedTaskHubWorker(); | ||
| this.config.ConfigureForGrpcProtocol(); | ||
|
|
||
| var attribute = this.CreateClientAttributeFromRequest(request); | ||
| IDurableOrchestrationClient client = this.GetClient(request, attribute); | ||
| var bindings = new BindingHelper(this.config); | ||
| string clientString = bindings.DurableOrchestrationClientToString(client, attribute); | ||
|
|
||
| return request.CreateResponse(HttpStatusCode.OK, clientString); | ||
| } |
There was a problem hiding this comment.
The new HandleConfigureGrpcRequest endpoint performs a significant configuration change (clearing cached task hub worker and reconfiguring protocol) but has no apparent authentication or authorization checks. This endpoint could potentially be exploited if exposed to unauthorized users, allowing them to disrupt service by repeatedly clearing the cached worker or changing the protocol configuration. Consider adding appropriate security measures to restrict access to this administrative endpoint, similar to how other management endpoints might be protected.
|
|
||
| private static TemplateMatcher GetConfigureGrpcRoute() | ||
| { | ||
| return new TemplateMatcher(TemplateParser.Parse($"{ManagementControllerSegment}{{{ConfigureGrpcRouteParameter}}}"), new RouteValueDictionary()); |
There was a problem hiding this comment.
The route template includes a route parameter {ConfigureGrpcRouteParameter} which is never accessed or used in the HandleConfigureGrpcRequest method. This creates an inconsistent API where the route expects /management/{configureGrpc} with a variable segment, but that value is ignored. Comparing with MakePrimaryRoute (line 187-189), which uses a fixed path without parameters, the ConfigureGrpc route should likely be a fixed path like /management/configureGrpc instead. Either remove the route parameter to make it a fixed path, or document why the parameter exists and what valid values it accepts.
| return new TemplateMatcher(TemplateParser.Parse($"{ManagementControllerSegment}{{{ConfigureGrpcRouteParameter}}}"), new RouteValueDictionary()); | |
| return new TemplateMatcher(TemplateParser.Parse($"{ManagementControllerSegment}{ConfigureGrpcRouteParameter}"), new RouteValueDictionary()); |
| if (taskHub is null | ||
| && key.Equals(TaskHubParameter, StringComparison.OrdinalIgnoreCase) | ||
| && !string.IsNullOrWhiteSpace(pairs[key])) | ||
| { | ||
| taskHub = pairs[key]; | ||
| } | ||
| else if (connectionName == null | ||
| else if (connectionName is null |
There was a problem hiding this comment.
The null comparison pattern has been changed from == null to is null. However, the codebase predominantly uses == null for null checks (see lines 367, 383, 590, 629, 675, 844, 868, 892, 1064, 1102, 1195). For consistency with the existing codebase conventions, consider using == null instead of is null.
|
Unsolved issue: the TriggerValueType for orchestrators is determined using OutOfProcProtocol set in ConfigureForGrpcProtocol(). In other use cases of ConfigureForGrpcProtocol (using function metadata) the protocol is set before TriggerValueType is accessed by WebJobs and there is no conflict, however, if this new endpoint is hit, WebJobs will continue using the wrong type for orchestration bindings. Need to come up with a way around this if this solution is to be properly considered |
Adds an HTTP endpoint that allows clients to configure the extension for gRPC. Useful in the case when function metadata is not a viable option for HTTP vs gRPC detection, like legacy function.json-based programming models without worker indexing.
Project checklist
pending_docs.mdrelease_notes.mdWebJobs.Extensions.DurableTaskpackage/src/Worker.Extensions.DurableTask/AssemblyInfo.csEventSourcelogsv2.xbranchWebJobs.Extensions.DurableTaskv3.x and will be retained only in thedevandmainbranchesAI-assisted code disclosure (required)
Was an AI tool used? (select one)
If AI was used:
AI verification (required if AI was used):
Testing
Automated tests
Manual validation (only if runtime/behavior changed)
1.
2.
3.
Notes for reviewers