Update position before ReadAsync starts, but fix it after incomplete read #56531
Conversation
|
Tagging subscribers to this area: @dotnet/area-system-io Issue Details
|
| @@ -99,6 +99,10 @@ public unsafe sealed override long Length | |||
| } | |||
| } | |||
|
|
|||
| // in case of concurrent incomplete reads, there can be multiple threads trying to update the position | |||
| // at the same time. That is why we are using Interlocked here. | |||
| internal void OnIncompleteRead(int expectedBytesRead, int actualBytesRead) => Interlocked.Add(ref _filePosition, actualBytesRead - expectedBytesRead); | |||
There was a problem hiding this comment.
@stephentoub I've used Interlocked here as the new IncompleteReadCantSetPositionBeyondEndOfFile test was simply failing without it. I know that we are never going to be 100% thread safe, but the alternative was to call Length which would result in an extra sys-call.
_filePosition = Math.Min(Length, _filePosition - expectedBytesRead + actualBytesRead);
|
the test is failing on Windows 7 and 8, I am going to apply no merge until I fix it |
adamsitnik
added
NO-MERGE
…dating file position before performing async write can lead to invalid cached length value
adamsitnik
force-pushed
the
readAsyncPositionChange
branch
from
a296615
to
30b5768
Compare
adamsitnik
force-pushed
the
readAsyncPositionChange
branch
from
e0a8bf4
to
c513ce5
Compare
adamsitnik
removed
the
NO-MERGE
|
Setting file position and length via I've disabled |
|
@stephentoub all tests are passing, PTAL |
| @@ -286,6 +288,13 @@ internal static unsafe (SafeFileHandle.OverlappedValueTaskSource? vts, int error | |||
| vts.Dispose(); | |||
| throw; | |||
| } | |||
| finally | |||
| { | |||
| if (errorCode != Interop.Errors.ERROR_IO_PENDING && errorCode != Interop.Errors.ERROR_SUCCESS) | |||
There was a problem hiding this comment.
Nit: this function is initializing errorCode to '0' but then checking against 'ERROR_SUCCESS'. Style-wise, it'd be nice if they were consistent.
| { | ||
| // Update Position... it could be anywhere. | ||
| _filePosition = positionBefore; | ||
| Interlocked.Exchange(ref _filePosition, positionBefore); | ||
| } |
There was a problem hiding this comment.
This is precarious regardless of the interlocked, e.g. if someone did kick off concurrent operations, they could all fight to reset this to their starting position. It's not clear there's a "right" answer.
ghost
locked as resolved and limited conversation to collaborators
|
Did this PR effectively negate the breaking change listed here? dotnet/docs#23858 |
Yes and no. IIRC we wanted to update the position after the operations are completed, but then we realized that some users use My main goal with dotnet/docs#23858 was to give the users a clear message: stop relying on that, don't use |
No description provided.