Skip to content

Conversation

@masaori335
Copy link
Contributor

@masaori335 masaori335 commented Sep 9, 2016

Below is the situation when this failure is happen.

  1. h2spec sends a HEADERS frame with END_HEADERS flag and without END_STREAM flag.
  2. TS returns a HEADERS frame and two DATA frames immediately. And TS set server side stream state closed.
  3. h2spec sends RST_STREAM with a length other than 4 cotets.
  4. TS ignores RST_STREAM to closed stream.

There are two problems.

  • h2spec assumes server side stream state is open. ( This is fixed by h2spec v1.5.0 )
  • At no. 2 , TS should change server side stream state to half-close (local).
    And send RST_STREAM frame to client and make state closed.

To fix this

  • Change stream state to half-close (local) from idle or open when send a frame w/ END_STREAM flag
  • Make send_a_data_frame to return HTTP2_SEND_A_DATA_FRAME_DONE when send DATA frame w/ END_STREAM flag
  • Set stream state CLOSED when error is happen

- Change stream state from IDLE or OPEN to HALF_CLOSED_LOCAL when send a frame w/ END_STREAM
- Make send_a_data_frame to return HTTP2_SEND_A_DATA_FRAME_DONE when send DATA frame w/ END_STREAM
- Set stream state CLOSED when error is happen
@atsci
Copy link

atsci commented Sep 9, 2016

FreeBSD build successful! See https://ci.trafficserver.apache.org/job/Github-FreeBSD/762/ for details.

@atsci
Copy link

atsci commented Sep 9, 2016

Linux build successful! See https://ci.trafficserver.apache.org/job/Github-Linux/658/ for details.

@shinrich
Copy link
Member

shinrich commented Sep 9, 2016

The changes look reasonable to me. Separate tracking sending EOS and receiving EOS seems like a good idea

@maskit
Copy link
Member

maskit commented Sep 9, 2016

Looks OK. I understand the necessity of doing this but I'm not thrilled adding more state flags to Http2Stream class. I can live with them but I hope they are managed in _state in the future.

@masaori335 masaori335 merged commit 35f9761 into apache:master Sep 12, 2016
@bryancall bryancall added this to the 7.0.0 milestone Jun 9, 2017
bneradt pushed a commit to bneradt/trafficserver that referenced this pull request Nov 26, 2024
…1823) (apache#999)

Co-authored-by: David Carlin <dcarlin@yahoo-inc.com>
(cherry picked from commit 9cf559a)

Co-authored-by: David Carlin <dcarlin@apache.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants