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.

Sign up for GitHub

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

Jump to bottom

Beware: Idle states #11

Closed
olix0r opened this issue Aug 2, 2017 · 8 comments
Closed

Beware: Idle states #11

olix0r opened this issue Aug 2, 2017 · 8 comments
Assignees
@hawkw
Milestone
v0.1

Comments

@olix0r
Copy link
Collaborator

olix0r commented Aug 2, 2017

I noticed the following:

The first use of a new stream identifier implicitly closes all streams in the "idle" state that might have been initiated by that peer with a lower-valued stream identifier. For example, if a client sends a HEADERS frame on stream 7 without ever sending a frame on stream 5, then stream 5 transitions to the "closed" state when the first frame for stream 7 is sent or received.

We need to be careful that we don't initialize streams in the idle state such that they may be moved into reserved/open out of order. We should probably only ever initialize streams directly to a Reserved or Open (or HalfOpenRemote) state.
@carllerche
Copy link
Collaborator

This issue can be closed once there is a test ensuring this behavior is followed. I am pretty sure this behavior is already implemented.

@carllerche carllerche added this to the v0.1 milestone Sep 18, 2017
@hawkw hawkw self-assigned this Oct 18, 2017
@olix0r
Copy link
Collaborator Author

olix0r commented Oct 18, 2017

To expand on my comment above: we need the client's send side (i.e. prioritize.rs) to ensure that headers are sent in stream-id-order.

@carllerche
Copy link
Collaborator

In fact, h2 probably should have an assert that stream id of initial headers frame is greater than all previously sent stream ids

hawkw added a commit that referenced this issue Oct 18, 2017
@hawkw
Add test for issue #11
ebbc67a
hawkw added a commit that referenced this issue Oct 19, 2017
@hawkw
Add test for issue #11
77025d0
@hawkw
Copy link
Collaborator

hawkw commented Oct 19, 2017

@carllerche for the assertion you mentioned, is it reasonable to use send.next_stream_id to track previously sent stream ids?

@carllerche
Copy link
Collaborator

@hawkw I think so...

@hawkw
Copy link
Collaborator

hawkw commented Oct 19, 2017

@carllerche okay, adding the assertion breaks a number of tests - it looks like it's mostly tests where a stream ID overflows, but since the assertion has to handle stream ID overflows, the panic happens in a different place. Is it reasonable for me to modify those tests?

@carllerche
Copy link
Collaborator

@hawkw Are you good here? I believe you followed up w/ @seanmonstar?

hawkw added a commit that referenced this issue Oct 20, 2017
@hawkw
Add test for issue #11
d004690
@hawkw hawkw mentioned this issue Oct 20, 2017
Merged
@hawkw
Copy link
Collaborator

hawkw commented Oct 20, 2017

@carllerche yeah, I talked it over with Sean and I've now opened a PR #160 to get this merged.

cxw620 pushed a commit to cxw620/h2 that referenced this issue Jan 20, 2025
@0x676e67 @seanmonstar
@dswij @Noah-Kennedy @niceskylei @jiahaoliang
Merge patch (hyperium#16)
8e21a74 
* v0.3.26

* Rename project to `rh2`

* Refactor frame sending custom implementation

* Export frame `PseudoOrder` settings

* Reduce unnecessary Option packaging

* v0.3.27

* fix(frame/headers): Fix error when headers priority is empty

* v0.3.29

* feat(frame/headers): Packaging headers pseudo order type (hyperium#8)

* feat(frame/settings): Packaging settings type (hyperium#9)

* Initialize frame settings order in advance

* v0.3.31

* feat(frame): Add unknown_setting frame settings (hyperium#10)

* Add unknown_setting patch

* Customize all Http Settings order

* v0.3.40

* fix(frame): Fix unknown setting encode (hyperium#11)

* v0.3.41

* feat: Replace with static settings (hyperium#12)

* v0.3.50

* feat: Destructive update, fixed-length array records the setting frame order (hyperium#13)

* v0.3.60

* Update README.md

* Sync upstream (hyperium#14)

* fix: streams awaiting capacity lockout (hyperium#730) (hyperium#734)

This PR changes the the assign-capacity queue to prioritize streams that
are send-ready.

This is necessary to prevent a lockout when streams aren't able to proceed while
waiting for connection capacity, but there is none.

Closes hyperium/hyper#3338

Co-authored-by: dswij <dharmasw@outlook.com>

* v0.3.23

* streams: limit error resets for misbehaving connections

This change causes GOAWAYs to be issued to misbehaving connections which for one reason or another cause us to emit lots of error resets.

Error resets are not generally expected from valid implementations anyways.

The threshold after which we issue GOAWAYs is tunable, and will default to 1024.

* Prepare v0.3.24

* perf: optimize header list size calculations (hyperium#750)

This speeds up loading blocks in cases where we have many headers already.

* v0.3.25

* refactor: cleanup new unused warnings (hyperium#757)

* fix: limit number of CONTINUATION frames allowed

Calculate the amount of allowed CONTINUATION frames based on other
settings.

    max_header_list_size / max_frame_size

That is about how many CONTINUATION frames would be needed to send
headers up to the max allowed size. We then multiply by that by a small
amount, to allow for implementations that don't perfectly pack into the
minimum frames *needed*.

In practice, *much* more than that would be a very inefficient peer, or
a peer trying to waste resources.

See https://seanmonstar.com/blog/hyper-http2-continuation-flood/ for
more info.

* v0.3.26

* fix: return a WriteZero error if frames cannot be written (hyperium#783)

Some operating systems will allow you continually call `write()` on a closed socket, and will return `Ok(0)` instead of an error. This patch checks for a zero write, and instead of looping forever trying to write, returns a proper error.

Closes hyperium#781

Co-authored-by: leibeiyi <leibeiyi@coocaa.com>

* lints: fix unexpected cfgs warnings

* ci: pin deps for MSRV

* ci: pin more deps for MSRV job (hyperium#817)

* fix: notify_recv after send_reset() in reset_on_recv_stream_err() to ensure local stream is released properly (hyperium#816)

Similar to what have been done in fn send_reset<B>(), we should notify RecvStream that is parked after send_reset().

Co-authored-by: Jiahao Liang <gzliangjiahao@gmail.com>

---------

Co-authored-by: Sean McArthur <sean@seanmonstar.com>
Co-authored-by: dswij <dharmasw@outlook.com>
Co-authored-by: Noah Kennedy <nkennedy@cloudflare.com>
Co-authored-by: beiyi lei <niceskylei@gmail.com>
Co-authored-by: leibeiyi <leibeiyi@coocaa.com>
Co-authored-by: Jiahao Liang <gzliangjiahao@gmail.com>

* v0.3.61

---------

Co-authored-by: Sean McArthur <sean@seanmonstar.com>
Co-authored-by: dswij <dharmasw@outlook.com>
Co-authored-by: Noah Kennedy <nkennedy@cloudflare.com>
Co-authored-by: beiyi lei <niceskylei@gmail.com>
Co-authored-by: leibeiyi <leibeiyi@coocaa.com>
Co-authored-by: Jiahao Liang <gzliangjiahao@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@hawkw hawkw

Labels
None yet
Projects
None yet
Milestone
v0.1
Development

No branches or pull requests
3 participants
@carllerche @olix0r @hawkw