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

Fix SNIPacket.ReadFromStreamAsync to not consume same ValueTask twice #295

Merged
merged 1 commit into from Nov 7, 2019

Conversation

stephentoub
Copy link
Member

@David-Engel
Copy link
Contributor

Nice find. Any ideas on when and how this would manifest itself to the application and how to add a test for it?

@stephentoub
Copy link
Member Author

stephentoub commented Nov 4, 2019

Any ideas on when and how this would manifest itself to the application and how to add a test for it?

I don't know at a high-level when this ReadFromStreamAsync method is invoked. When it is invoked, though, if:

  • The read on the underlying stream completed synchronously or otherwise really fast such that it was already completed by the time we check, and
  • If the result "length" was <= 0 (which assuming a well behaving stream would mean 0 and that we hit EOF), and
  • If the underlying stream was implemented with pooled IValueTaskSource instances (uncommon right now, more common in the future)

then it's possible we could end up using an instance backed by a pooled object already being used concurrently by someone else... most likely outcome would be this caller getting an InvalidOperationException stating that the object was being reused incorrectly.

So, it's unlikely to be an impactful issue, and also relatively hard to test well. But worth fixing in my opinion regardless.

@David-Engel David-Engel added this to the 1.1.0 milestone Nov 7, 2019
@David-Engel David-Engel merged commit b343b01 into dotnet:master Nov 7, 2019
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.

None yet

2 participants