Add test to read and process json from pipe #2550
Conversation
| { | ||
| ReadResult result = await _pipe.Reader.ReadAsync(); | ||
| ReadOnlySequence<byte> data = result.Buffer; | ||
| bool isFinalBlock = data.Length == 0; // TODO: Fix isFinalBlock condition, if Writer is done instead, isLastSegment? |
There was a problem hiding this comment.
This isn't the right condition to set isFinalBlock. It needs to be based on whether Writer has completed or not.
I was thinking of using IsLastSegment (making it a field), but I can only set IsLastSegment after writing the last segment, and that could lead to a race where the reader reads the data before I set IsLastSegment to true.
Maybe something related to _pipe.Reader.OnWriterCompleted() can tell me if isFinalBlock should be set or not.
Any suggestions?
There was a problem hiding this comment.
data.Length == 0 && result.IsCompleted
There was a problem hiding this comment.
That condition results in an extra (unnecessary) call to Read with a knowingly empty json payload. It also doesn't catch invalid json scenarios like these:
Single segment : 1234. <= should throw since the number ends with a decimal point.
We don't know isFinalBlock is true, after the decimal we return false. So, data.Length is never == 0, and we never get isFinalBlock is true.
Should it be as follows instead?
bool isFinalBlock = result.IsCompleted;
Doing so solves the correctness issue that I stated above.
tools/dependencies.props
Outdated
| <SystemCompilerServicesUnsafeVersion>4.6.0-preview1-27011-05</SystemCompilerServicesUnsafeVersion> | ||
| <SystemNumericsVectorsVersion>4.6.0-preview1-27011-05</SystemNumericsVectorsVersion> | ||
| <SystemBuffersVersion>4.6.0-preview1-26912-03</SystemBuffersVersion> | ||
| <SystemIOPipelinesVersion>4.6.0-preview1-27011-05</SystemIOPipelinesVersion> |
There was a problem hiding this comment.
Referencing the latest package is failing with:
[10/20/2018 3:09:24 AM Error] [xUnit.net 00:00:25.5567575] System.Text.JsonLab.Tests.ReadFromPipeTests.ReadFromPipe [FAIL]
[10/20/2018 3:09:24 AM Informational] [xUnit.net 00:00:25.5585306] System.IO.FileNotFoundException : Could not load file or assembly 'System.Buffers, Version=4.0.4.0, Culture=neutral, PublicKeyToken=cc7b13ffcd2ddd51'. The system cannot find the file specified.
[10/20/2018 3:09:24 AM Informational] [xUnit.net 00:00:25.5598690] Stack Trace:
[10/20/2018 3:09:24 AM Informational] [xUnit.net 00:00:25.5608873] at System.IO.Pipelines.PipeCompletion.GetCallbacks()
[10/20/2018 3:09:24 AM Informational] [xUnit.net 00:00:25.5609648] at System.IO.Pipelines.Pipe.CompleteWriter(Exception exception)
[10/20/2018 3:09:24 AM Informational] [xUnit.net 00:00:25.5610360] E:\GitHub\Fork\corefxlab\tests\System.Text.JsonLab.Tests\ReadFromPipeTests.cs(25,0): at System.Text.JsonLab.Tests.ReadFromPipeTests.Dispose()
[10/20/2018 3:09:24 AM Informational] [xUnit.net 00:00:25.5614956] at ReflectionAbstractionExtensions.DisposeTestClass(ITest test, Object testClass, IMessageBus messageBus, ExecutionTimer timer, CancellationTokenSource cancellationTokenSource)
[10/20/2018 3:09:24 AM Informational] [xUnit.net 00:00:25.5751005] Finished: System.Text.JsonLab.Tests
cc @pakrym
|
@dotnet-bot test this please |
|
The tests are expected to fail until we get this issue resolved: https://github.com/dotnet/corefx/issues/32984 |
|
Waiting on a new build which has this fix: dotnet/corefx#32992 |
|
Superseded by #2570. Closing. |
No description provided.