[Service Bus] Add timeout to shutdown#120
Conversation
loic-sharma
requested review from
agr,
chenriksson,
cristinamanum,
dtivel,
joelverhagen,
shishirx34 and
skofman1
| public async Task StopReturnsTrueIfClientCloseAsyncMethodFinishesFirst() | ||
| { | ||
| // Arrange | ||
| _client.Setup(c => c.CloseAsync()).Returns(Task.Delay(TimeSpan.FromMilliseconds(5))); |
There was a problem hiding this comment.
Does it even need a 5ms sleep, or can we reduce to 1 or 0?
There was a problem hiding this comment.
Ideally we'd use TimeSpan.MaxValue and TimeSpan.MinValue, but, Task.Delay was throwing exceptions. A human cannot tell the difference between 1ms or 5ms, but sure.
| // Start two tasks: one for the shutdown and one for the shutdown's timeout. The first task to finish will | ||
| // complete the "start shutdown" operation. Note that this method returns false if the timeout task completes first. | ||
| var shutdownTask = _client.CloseAsync(); | ||
| var shutdownTimeoutTask = Task.Delay(timeout); |
There was a problem hiding this comment.
This isn't really a timeout for CloseAsync, just a timeout for how long to wait.
It looks like there's a ServiceBus OperationTimeout, settable via the messaging factory. I wonder if that's really the approach we should be taking. Found the timeout via reflector, but see also stackoverflow:
https://stackoverflow.com/questions/24416435/setting-the-operationtimeout-property-for-a-service-bus-worker-role
There was a problem hiding this comment.
That is correct - it is a timeout on how long to wait. Let me know if you think the documentation on the interface for this method is confusing.
It seems that OperationTimeout does not relate to the time the CloseAsync method takes, but rather, the time a thread can take to process a message. This fix is still necessary (note that this is copying code that already exists in the Orchestrator)
Also, we lease the messages from Service Bus. The handlers will fail to update the state of a message whose lease has expired.
There was a problem hiding this comment.
Documentation is clear. Assuming the process will just exit if we don't block on a hung CloseAsync?
Re: OperationTimeout does not relate to the time the CloseAsync method takes...
This is what I was wondering; whether OperationTimeout would prevent a CloseAsync hang due to processing of messages. Looks like it defaults to 1 min, so makes sense if this isn't related.
Seems ok for now, but do we know why CloseAsync hangs? Just want to make sure we clean up properly and don't have side effects.
There was a problem hiding this comment.
Kind of... the process won't end if we're blocked on CloseAsync, but, other factors may prevent the process from exiting.
We're not sure what causes the CloseAsync hang, @agr: is still investigating.
|
|
||
| private async Task OnMessageAsync(IBrokeredMessage brokeredMessage) | ||
| { | ||
| if (_running == false) |
The core logic of Service Bus shutdown was moved to the Service Bus dependencies. This pull request removes the shutdown logic that Service Bus dependency now handles. Part of https://github.com/NuGet/Engineering/issues/1115 Depends on NuGet/ServerCommon#120
The core logic of Service Bus shutdown was moved to the Service Bus dependencies. This pull request removes the shutdown logic that Service Bus dependency now handles. Part of https://github.com/NuGet/Engineering/issues/1115 Depends on NuGet/ServerCommon#120
3 participants
Today, the Orchestrator has custom code to ensure that:
This brings the logic to the
SubscriptionProcessorso that all other jobs can also have these guarantees.Part of https://github.com/NuGet/Engineering/issues/1115