GOAWAY race with more strict stream handling #681

Closed
mattklein123 opened this Issue Sep 9, 2016 · 5 comments

Projects

None yet

2 participants

@mattklein123

Hi,

We are upgrading from 1.9.2 to 1.14.0. I'm having an issue with this change:
16c4611#diff-56caa9d28caad2d74e6a938e947f81b8R5133

The following sequence of events happens:

  1. Server sends GOAWAY (via nghttp2_submit_goaway)
  2. GOAWAY races with incoming new stream
  3. In previous version of library, new stream (and further data frames for the stream) would be ignored by the library
  4. Now because of this change, it appears that the data frame(s) for the raced streams are assumed to part of an unknown stream, which then end up generating a connection level error. This prevents graceful completion of still inflight streams.

I realize that I should be doing graceful shutdown via nghttp2_submit_shutdown_notice(), but that is a large change which I would prefer to not do right now.

This feels like a regression to me (especially since the header frame for the new stream appears to be ignored), but I am not familiar enough with the spec to know for sure.

What do you think about this?

Thanks,
Matt

@tatsuhiro-t
Collaborator

You are right, that commit introduced race. I figured another case that it cannot handle well.
To fix this issue, I reverted part of the commit.
Fix committed via 6858cda

@tatsuhiro-t tatsuhiro-t added this to the v1.15.0 milestone Sep 9, 2016
@mattklein123

Thanks for the quick response. If possible, it would be great if you could release a 1.14.1 with this fix as it is causing us production issues. If that's a big deal I can manually apply the patch.

Thanks,
Matt

@tatsuhiro-t
Collaborator

Will do. The next release is 25th this month, which is 2 weeks later, a bit too long to wait.

@mattklein123

Awesome thanks.

@tatsuhiro-t tatsuhiro-t modified the milestone: v1.14.1, v1.15.0 Sep 10, 2016
@tatsuhiro-t
Collaborator

Fixed in v1.14.1.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment