CSHARP-5777: Avoid ThreadPool-dependent IO methods in sync API #1805
Conversation
sanych-sun
requested review from
BorisDog and
ajcvickers
and removed request for
a team and
ajcvickers
BorisDog
left a comment
BorisDog
left a comment
There was a problem hiding this comment.
Looks good, few small comments.
|
|
||
| cancellationToken.ThrowIfCancellationRequested(); | ||
| throw new TimeoutException(); | ||
| return UseStreamWithTimeout( |
There was a problem hiding this comment.
Few naming suggestions:
UseStreamWithTimeout -> rename to ExecuteOperationWithTimeout?
method -> rename to operation?
| } | ||
| } | ||
|
|
||
| private static TResult UseStreamWithTimeout<TResult, TState>(Stream stream, Func<Stream, TState, TResult> method, TState state, TimeSpan timeout, CancellationToken cancellationToken) |
There was a problem hiding this comment.
Maybe just
byte[] buffer, int offset, int count instead of state, as it's the same in both cases?
There was a problem hiding this comment.
I've switched to using ValueTuple, otherwise int parameters of the Func operation might be ambiguous.
|
The latest changes build: https://spruce.mongodb.com/version/6912484caf54450007c31989 |
| var disposeCallbackState = (StreamDisposeCallbackState)state; | ||
| if (!disposeCallbackState.TryChangeStateFromInProgress(OperationState.Cancelled)) | ||
| { | ||
| // if cannot change the state - then I/O was already succeeded |
There was a problem hiding this comment.
minor: "It the state can't be changed- then I/O had already succeeded"
| catch (IOException) | ||
| { | ||
| if (callbackState?.OperationState == OperationState.Cancelled) | ||
| { |
There was a problem hiding this comment.
Please add a comment explaining that Cancelled means both cancelationtoken cancelation and timeout.
|
|
||
| private static TResult ExecuteOperationWithTimeout<TResult>(Stream stream, Func<Stream, (byte[] Buffer, int Offset, int Count), TResult> operation, byte[] buffer, int offset, int count, TimeSpan timeout, CancellationToken cancellationToken) | ||
| { | ||
| StreamDisposeCallbackState callbackState = null; |
There was a problem hiding this comment.
Do we have tests to test all the major cases here? Timeout, cancellation, success.
|
|
||
| cancellationToken.ThrowIfCancellationRequested(); | ||
| throw new TimeoutException(); | ||
| return ExecuteOperationWithTimeout( |
There was a problem hiding this comment.
minor: can use expression body.
| manualResetEvent); | ||
|
|
||
| if (writeOperation.IsCompleted || manualResetEvent.Wait(timeout, cancellationToken)) | ||
| ExecuteOperationWithTimeout( |
There was a problem hiding this comment.
minor: can use expression body.
2 participants
The PR is created based on 3.5.x branch, here is the custom EG patch to validate it's green:
https://spruce.mongodb.com/version/69052114d023dd0007b247d7