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

Refactor http2 #423

Merged
merged 8 commits into from Aug 13, 2015

Conversation

Projects
None yet
4 participants
@kazu-yamamoto
Contributor

kazu-yamamoto commented Aug 12, 2015

  1. With this patches, Priority is included in Stream. Prirority of a stream now can be updated by the priority frame.
  2. The old code checks stream window size after dequeuing. If the size is 0, it is enqueued again. This is redundant and inefficient. So, the new code check it before enqueuing.

@awpr @AaronFriel Would you take time to review this patches?

kazu-yamamoto added some commits Aug 11, 2015

enqueuing response of streams whose window size is greater than 0.
INVARIANT: streams in the output queue have non-zero window size.
Old: dequeue, check window, forkIO, wait and then enqueue again.
New: check window, forkIO, wait and then enqueue.
@awpr

This comment has been minimized.

Show comment
Hide comment
@awpr

awpr Aug 12, 2015

Member

Looks good. It might make sense to add another function alongside 'enqueueWhenWindowIsOpen' to check and enqueue immediately or forkIO and enqueue later, since that logic seems to be duplicated between 'maybeEnqueueNext' and 'response'.

Could you add a comment on the new purpose of 'checkWindowSize'? It took me a while to figure out that it actually did intend to block the sender thread until the connection window was nonzero. It might actually be clearer now to un-CPS it since it unconditionally calls 'body' now.

Member

awpr commented Aug 12, 2015

Looks good. It might make sense to add another function alongside 'enqueueWhenWindowIsOpen' to check and enqueue immediately or forkIO and enqueue later, since that logic seems to be duplicated between 'maybeEnqueueNext' and 'response'.

Could you add a comment on the new purpose of 'checkWindowSize'? It took me a while to figure out that it actually did intend to block the sender thread until the connection window was nonzero. It might actually be clearer now to un-CPS it since it unconditionally calls 'body' now.

@kazu-yamamoto

This comment has been minimized.

Show comment
Hide comment
@kazu-yamamoto

kazu-yamamoto Aug 13, 2015

Contributor

@awpr Now looks better?

Contributor

kazu-yamamoto commented Aug 13, 2015

@awpr Now looks better?

@awpr

This comment has been minimized.

Show comment
Hide comment
@awpr

awpr Aug 13, 2015

Member

Looks good.

A separate issue that wasn't introduced by this pull request but I may as well bring up here: if the sender gets an Output for a closed stream it will terminate rather than just skipping it, because the tail call to 'loop' is inside the 'unlessClosed' in 2 or 3 cases.

Member

awpr commented Aug 13, 2015

Looks good.

A separate issue that wasn't introduced by this pull request but I may as well bring up here: if the sender gets an Output for a closed stream it will terminate rather than just skipping it, because the tail call to 'loop' is inside the 'unlessClosed' in 2 or 3 cases.

@kazu-yamamoto

This comment has been minimized.

Show comment
Hide comment
@kazu-yamamoto

kazu-yamamoto Aug 13, 2015

Contributor

@awpr Thank you for you review. I will merge this pull request by myself.

For the closed stream issue, would you send a pull request?

Contributor

kazu-yamamoto commented Aug 13, 2015

@awpr Thank you for you review. I will merge this pull request by myself.

For the closed stream issue, would you send a pull request?

kazu-yamamoto added a commit that referenced this pull request Aug 13, 2015

@kazu-yamamoto kazu-yamamoto merged commit 9c4ac89 into master Aug 13, 2015

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@snoyberg snoyberg removed the in progress label Aug 13, 2015

@kazu-yamamoto kazu-yamamoto deleted the refactor-http2 branch Aug 13, 2015

@AaronFriel

This comment has been minimized.

Show comment
Hide comment
@AaronFriel

AaronFriel Aug 13, 2015

Contributor

@kazu-yamamoto I'm sorry I didn't get a chance to look at this, I will defer to @awpr on all things HTTP/2.0. He's got the right ideas and the time, and I'm not yet familiar enough with wai and warp to comment on big pieces like this.

I am excited to see more HTTP/2.0 support merged in, however!

Contributor

AaronFriel commented Aug 13, 2015

@kazu-yamamoto I'm sorry I didn't get a chance to look at this, I will defer to @awpr on all things HTTP/2.0. He's got the right ideas and the time, and I'm not yet familiar enough with wai and warp to comment on big pieces like this.

I am excited to see more HTTP/2.0 support merged in, however!

@awpr

This comment has been minimized.

Show comment
Hide comment
@awpr

awpr Aug 13, 2015

Member

@kazu-yamamoto Working on it -- it was an easy change to make, but my test server depends on my HTTP2Application changes, and it's a time-consuming merge with this pull.

Member

awpr commented Aug 13, 2015

@kazu-yamamoto Working on it -- it was an easy change to make, but my test server depends on my HTTP2Application changes, and it's a time-consuming merge with this pull.

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