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 |
|
|
||
| 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.
| var stream = streamMock.Object; | ||
|
|
||
| var destination = new byte[2]; | ||
| using var cancellationTokenSource = new CancellationTokenSource(10); |
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