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

iostream: Resolve pending reads on stream close #2805

Merged
merged 5 commits into from
Feb 1, 2020
Merged

Conversation

bdarnell
Copy link
Member

@bdarnell bdarnell commented Feb 1, 2020

Includes code from #2719 and tests from #2720.

Fixes two bugs related to interactions between reads and closes:

  • Reads which started before the close can now complete from data read into the buffer before the stream closed.
  • Reads which start after the close can also use buffered data instead of immediately raising an error.

Fixing the latter issue narrowed the scope of the change introduced in #2670, so we should watch for a recurrence of #2651. We haven't been able to construct good test cases for #2651 or the second issue fixed here; the timing required is subtle (although we have non-unittest reproductions).

Closes #2717
Closes #2719
Closes #2720

minrk and others added 4 commits February 1, 2020 12:22
fixes issue that a read may fail with StreamClosedError
if stream is closed mid-read
_start_read can resolve with _try_inline_read, which can succeed even if the stream has been closed
if the buffer has been populated by a prior read

preserve the fix for asserts being hit when dealing with closed sockets
@bdarnell bdarnell merged commit f7d94d0 into master Feb 1, 2020
@bdarnell bdarnell deleted the resolve-pending branch February 1, 2020 21:44
@lisongmin
Copy link

Hi, @bdarnell

We have this problem randomly in product (single thread with multiple request concurrent). Should the MR merge to v6.0.4 at the same time? thanks.

@bdarnell
Copy link
Member Author

bdarnell commented Feb 4, 2020

Yeah, this should probably be backported to 6.0.x. But since this issue (and the related #2651) have proven so difficult to test I'd like to get reports from people who have tested this change in other releases (either from master or in 6.1 when that comes out) before I put it in a patch 6.0.x release.

Have you tried master? Does it work for you?

@lisongmin
Copy link

Sorry for delay.

There is no "Stream Closed" error after apply this patch so far. It works well for me.

@bdarnell
Copy link
Member Author

bdarnell commented Mar 3, 2020

Thanks for testing! I've decided to get this in to 6.0.4, which I'm preparing in #2819.

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.

IOStream read can fail if closed with an open read buffer
3 participants