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

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
@hawkw
Labels
enhancement

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 hawkw mentioned this issue Feb 4, 2021
Merged
hawkw added a commit that referenced this issue Feb 5, 2021
@hawkw
add Connection::max_concurrent_send_streams (#513)
978c712 
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
@hawkw
add Connection::max_concurrent_recv_streams
a72f7ea 
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
@hawkw
add Connection::max_concurrent_recv_streams
eb34f08 
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 mentioned this issue Feb 5, 2021
Merged
hawkw added a commit that referenced this issue Feb 19, 2021
@hawkw
add Connection::max_concurrent_recv_streams
610f167 
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
@hawkw
add Connection::max_concurrent_recv_streams (#516)
c1b411f 
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
@hawkw
add Connection::max_concurrent_send_streams (hyperium#513)
bae66c8 
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
@hawkw
add Connection::max_concurrent_recv_streams (hyperium#516)
b20d6c8 
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 •
edited

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();

@Yneth Yneth linked a pull request Oct 14, 2022 that will close this issue
Open
@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
Assignees

@hawkw hawkw

Labels
enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

add wait for remote settings being set before sending requests Yneth/h2
4 participants
@seanmonstar @hawkw @LucioFranco @Yneth