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

add Connection::max_concurrent_recv_streams #516

Merged
merged 2 commits into from
Feb 25, 2021

Conversation

hawkw
Copy link
Collaborator

@hawkw hawkw commented Feb 5, 2021

This commit adds accessors to client::Connection and
server::Connection that return the current value of the
SETTINGS_MAX_CONCURRENT_STREAMS limit that has been sent by this peer
and acknowledged by the remote.

This is analogous to the max_concurrent_send_streams methods added in
PR #513. These accessors may be somewhat less useful than the ones for
the values negotiated by the remote, since users who care about this
limit are probably setting the builder parameter. However, it seems
worth having for completeness sake --- and it might be useful for
determining whether or not a configured concurrency limit has been acked
yet...

Part of #512

@hawkw hawkw requested a review from seanmonstar February 5, 2021 18:06
@hawkw hawkw self-assigned this Feb 5, 2021
Copy link
Member

@seanmonstar seanmonstar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, just 1 docs fix needed.

/// [`SETTINGS_MAX_CONCURRENT_STREAMS` parameter][settings] in a `SETTINGS`
/// frame. This method returns the currently acknowledged value recieved
/// from the remote.
/// [`SETTINGS_MAX_CONCURRENT_STREAMS` parameter][1] in a `SETTINGS` frame.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this [1] change is half-way. The [settings]: .... below needs to be updated as well.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh whoops --- good catch!

This commit adds accessors to `client::Connection` and
`server::Connection` that return the current value of the
`SETTINGS_MAX_CONCURRENT_STREAMS` limit that has been sent by this peer
and acknowledged by the remote.

This is analogous to the `max_concurrent_send_streams` methods added in
PR #513. These accessors may be somewhat less useful than the ones for
the values negotiated by the remote, since users who care about this
limit are probably setting the builder parameter. However, it seems
worth having for completeness sake --- and it might be useful for
determining whether or not a configured concurrency limit has been acked
yet...

Part of #512
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
@hawkw hawkw force-pushed the eliza/expose-recv-streams branch from ab19270 to b91d5d5 Compare February 19, 2021 17:21
@seanmonstar seanmonstar merged commit c1b411f into master Feb 25, 2021
@seanmonstar seanmonstar deleted the eliza/expose-recv-streams branch February 25, 2021 16:58
hawkw added a commit to hawkw/hyper that referenced this pull request Mar 10, 2021
Upstream PRs hyperium/h2#513 and hyperium/h2#516 added functions for
exposing the connection concurrency limits negotiated by a remote peer.
Recording these settings can be useful for debugging purposes.

In the higher-level `Client` and `Server` APIs, it's not easily possible to
provide a way for users to directly access settings on a particular
connection, since the client abstracts over a connection pool and the
server abstracts over a connect loop. Therefore, the user can't access
these settings directly. Instead, this branch adds new log messages at
the DEBUG level for recording negotiated concurrency limits.

In a follow-up, we can also add accessors to the `Connection` types in
the lower-level `client::conn` and `server::conn` APIs.
hawkw added a commit to hawkw/hyper that referenced this pull request Mar 10, 2021
Upstream PRs hyperium/h2#513 and hyperium/h2#516 added functions for
exposing the connection concurrency limits negotiated by a remote peer.
Recording these settings can be useful for debugging purposes.

In the higher-level `Client` and `Server` APIs, it's not easily possible to
provide a way for users to directly access settings on a particular
connection, since the client abstracts over a connection pool and the
server abstracts over a connect loop. Therefore, the user can't access
these settings directly. Instead, this branch adds new log messages at
the DEBUG level for recording negotiated concurrency limits.

In a follow-up, we can also add accessors to the `Connection` types in
the lower-level `client::conn` and `server::conn` APIs.
BenxiangGe pushed a commit to BenxiangGe/h2 that referenced this pull request Jul 26, 2021
This commit adds accessors to `client::Connection` and
`server::Connection` that return the current value of the
`SETTINGS_MAX_CONCURRENT_STREAMS` limit that has been sent by this peer
and acknowledged by the remote.

This is analogous to the `max_concurrent_send_streams` methods added in
PR hyperium#513. These accessors may be somewhat less useful than the ones for
the values negotiated by the remote, since users who care about this
limit are probably setting the builder parameter. However, it seems
worth having for completeness sake --- and it might be useful for
determining whether or not a configured concurrency limit has been acked
yet...

Part of hyperium#512
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.

2 participants