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

expose information about negotiated connection settings #512

Open
hawkw opened this issue Feb 3, 2021 · 5 comments · May be fixed by #641
Open

expose information about negotiated connection settings #512

hawkw opened this issue Feb 3, 2021 · 5 comments · May be fixed by #641
Assignees

Comments

@hawkw
Copy link
Collaborator

hawkw commented Feb 3, 2021

h2 currently permits configuring most of the HTTP/2 protocol settings that are negotiated with the remote peer; for example, the client builder can configure settings like maximum frame size, maximum concurrent streams, etc.

However, once a connection is established, there's no way to access information about the settings that were negotiated with a remote peer, besides information about stream flow control. For example, in my particular use case, we would like to be able to record the max concurrent streams limit set by the server after initiating a client connection. It would be great to add some kind of API for querying connection-level settings that were received from a remote peer.

@hawkw hawkw self-assigned this Feb 3, 2021
@hawkw
Copy link
Collaborator Author

hawkw commented Feb 3, 2021

I'd be happy to work on this, but I'd love to get input on API design --- cc @seanmonstar.

@seanmonstar
Copy link
Member

Seems reasonable. My first instinct is to add getters to Connection.

@hawkw
Copy link
Collaborator Author

hawkw commented Feb 4, 2021

Seems reasonable. My first instinct is to add getters to Connection.

Hmm, that was my initial thought as well. I suppose the downside is that the Connection type is usually spawned immediately, so users need to access that information immediately. Probably not a huge issue.

hawkw added a commit that referenced this issue Feb 5, 2021
This PR adds accessors to `client::Connection` and `server::Connection`
that return the send stream concurrency limit on that connection, as
negotiated by the remote peer. This is part of issue #512.

I think we probably ought to expose similar accessors for other
settings, but I thought it was better to add each one in a separate,
focused PR.

Signed-off-by: Eliza Weisman <eliza@buoyant.io>
hawkw added a commit that referenced this issue 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 added a commit that referenced this issue 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 added a commit that referenced this issue Feb 19, 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
seanmonstar pushed a commit that referenced this issue Feb 25, 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
BenxiangGe pushed a commit to BenxiangGe/h2 that referenced this issue Jul 26, 2021
This PR adds accessors to `client::Connection` and `server::Connection`
that return the send stream concurrency limit on that connection, as
negotiated by the remote peer. This is part of issue hyperium#512.

I think we probably ought to expose similar accessors for other
settings, but I thought it was better to add each one in a separate,
focused PR.

Signed-off-by: Eliza Weisman <eliza@buoyant.io>
BenxiangGe pushed a commit to BenxiangGe/h2 that referenced this issue 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
@Yneth
Copy link

Yneth commented Oct 14, 2022

current implementation still does not return the remote max_send_streams.
I am not a maintainer but see two ways to solve it:

  • provide state machine API for Connection when the negotiated settings are received, example:
let (h2, connection) = client::Builder::new().initial_max_send_streams(1000000).handshake(tcp).await?;
let settings_negotiated = connection.await.unwrap();
tokio::spawn(async move {
   settings_negotiated.await.unwrap();
});
  • or somehow include such functionality in h2.ready(), example:
let (h2, connection) = client::Builder::new().initial_max_send_streams(1000000).handshake(tcp).await?;
tokio::spawn(async move {
   connection.await.unwrap();
});
let mut h2 = h2.ready().await?;
// in current implementation, max_send_streams may not be populated at this particular line
// adding tokio::time::sleep() helps, but it is ugly
h2.max_send_streams();

@LucioFranco
Copy link
Member

@Yneth maybe it makes sense to use ready as a signal that the data is ready then use global getters that return an Option<&ConfigItem>. Could maybe use something like a oncecell?

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

Successfully merging a pull request may close this issue.

4 participants