Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fixed issue #40 regarding partial streams #185

Merged
merged 4 commits into from
Oct 29, 2020
Merged

Conversation

Ralf1108
Copy link
Contributor

@Ralf1108 Ralf1108 commented Oct 7, 2020

No description provided.

@Ralf1108
Copy link
Contributor Author

Ralf1108 commented Oct 7, 2020

All tests are green and the PR Validation is wrong?
Can somebody give me a hint?
The error output is just:

Test Run Successful.
Total tests: 163
Passed: 162
Skipped: 1
Total time: 3.5337 Seconds
Running build failed.

  1. Fake.UnitTestCommon+FailedTestsException: xUnit2 reported an error (Error Code 1)

at [email protected](String m) in D:\a\1\s\build.fsx:line 82
at Microsoft.FSharp.Core.OptionModule.Iterate[T](FSharpFunc2 action, FSharpOption1 option)
at FSI_0005.Build.ResultHandling.failBuildIfXUnitReportedError@86.Invoke(FSharpOption1 option) in D:\a\1\s\build.fsx:line 86 at Microsoft.FSharp.Collections.SeqModule.Iterate[T](FSharpFunc2 action, IEnumerable1 source) at [email protected](Unit _arg1) in D:\a\1\s\build.fsx:line 108 at Fake.TargetHelper.runSingleTarget(TargetTemplate1 target) in C:\code\fake\src\app\FakeLib\TargetHelper.fs:line 626
##[error]Process completed with exit code 42.

@Aaronontheweb
Copy link
Member

Added @Arkatufus to this issue

@Arkatufus
Copy link
Contributor

Arkatufus commented Oct 26, 2020

The problem is with the current dotnet compiler for netcoreapp2.1, it requires a very arbitrary version of SDK to be installed, and it depends on the (most probably) msbuild installation.
It insisted on using 2.1.21 on my machine, but insisted on using 2.1.13 in azure pipelines.
The accepted solution is to drop testing for netcoreapp2.1.

@Aaronontheweb
Copy link
Member

Tests are passing now - just need to review. @Arkatufus do you want to take it or should I do it?-

@Arkatufus
Copy link
Contributor

I'm doing it right now

src/Hyperion/Extensions/StreamEx.cs Outdated Show resolved Hide resolved
src/Hyperion/Extensions/StreamEx.cs Show resolved Hide resolved
src/Hyperion/Extensions/StreamEx.cs Show resolved Hide resolved
@Arkatufus
Copy link
Contributor

@Aaronontheweb I will need your input on this.

@Arkatufus
Copy link
Contributor

Arkatufus commented Oct 26, 2020

The BinaryReader class guarantees any primitive bytes read for up to 16 bytes in length, ie. it will throw an unexpected EOF exception if the stream is interrupted mid-read. We can wrap all primitive reads with a BinaryReader to prevent the original issue from happening on primitive reads.

Our biggest concern is actually the multi-byte reads on the stream, such as arrays, strings, etc. We might need a guard on these reads, but I couldn't decide whether we should throw on partial fail reads or silently fail it.

@Aaronontheweb
Copy link
Member

@Arkatufus I'll need to revisit the issue - if this is a partial read issue then that might be the responsibility of the framing system to solve it, rather than something the serializer needs to handle.

@Aaronontheweb Aaronontheweb self-assigned this Oct 26, 2020
@Aaronontheweb Aaronontheweb self-requested a review October 26, 2020 21:29
@Arkatufus
Copy link
Contributor

Arkatufus commented Oct 26, 2020

I guess it really depends on type of deserialization is currently happening? We only need to guard on atomic primitive reads, but not on unknown length reads? if that is the case, then simply wrapping all Stream.Read with BinaryReader will fix this issue, at a cost.
Deserialize throughput will suffer a bit, especially on messages with a lot of reads.

Copy link
Member

@Aaronontheweb Aaronontheweb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need to handle the 0 bytes received case, but otherwise this PR looks good

src/Hyperion/Extensions/StreamEx.cs Outdated Show resolved Hide resolved
@Ralf1108
Copy link
Contributor Author

added default behavior to throw an EOF exception.

If the use case for useful partial reads comes up a flag to swallow the exception could be added.

@Arkatufus Arkatufus merged commit 81ff792 into akkadotnet:dev Oct 29, 2020
@Arkatufus
Copy link
Contributor

Thank you for the PR 👍

@Aaronontheweb Aaronontheweb mentioned this pull request Mar 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants