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

Stream Multiplexing and Flow Control (updated) #15

Merged
merged 11 commits into from Jun 15, 2019

Conversation

GallaFrancesco
Copy link
Contributor

Updated following the discussion in #14.

Changes from previous discussion:

  • HTTP2Multiplexer structures are not kept in a global table but are part of HTTP2ServerContext (one per connection -> one per context) and wrapped in a FreeListRef structure. It is passed by reference (using ref) across the http2.d and exchange.d modules.
  • IndexingTable management follows the same approach: removed the raw pointer across the HPACK files to support passing the table by reference, storing it in HTTP2ServerContext.

The commits have been rebased for better readability. Let me know if this needs to be split.
Thanks!

@s-ludwig
Copy link
Member

Thanks for reorganizing the commits, the basic structure looks good, although I didn't have enough time to go through everything in detail, and don't think I will any time soon, because it requires a contiguous time frame that I rarely have available these days.

So I think realistically we just need to switch the review mode. I'll merge the code in its current form and will try to dedicate a little time every once in a while to look through the repository and find possible places of improvement, opening appropriate PRs that you can review in turn. Hopefully, since the biggest parts are in place, the size of individual PRs is now also at a point where it can become much more limited, allowing for a more "casual" review mode.

@s-ludwig s-ludwig merged commit b9678d3 into vibe-d:master Jun 15, 2019
@GallaFrancesco
Copy link
Contributor Author

Thank you for the time you are putting into this, I know it can take some time (mine is quite limited too these days) and I'm fine with the review mode you proposed. I still have to work through the final additions such as stream prioritization, so taking some time to work on improvements now seems like the best choice.

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.

None yet

2 participants