Add `stream_id` accessors to public API types #292

hawkw · 2018-07-09T18:14:45Z

Problem:

Applications may want to access the underlying h2 stream ID for
diagnostics, etc. Stream IDs were not previously exposed in public APIs.

Solution:

The frame::StreamId type is now publically exported and has RustDoc
comments. The public API types SendStream, RecvStream,
ReleaseCapacity, client::ResponseFuture, and server::SendResponse
now all have stream_id methods which return the stream ID of the
corresponding stream.

Closes #289.

Signed-off-by: Eliza Weisman eliza@buoyant.io

This will be necessary to add similar accessors to the public API types that wrap these. Signed-off-by: Eliza Weisman <eliza@buoyant.io>

The types `SendStream`, `RecvStream`, `ReleaseCapacity`, `client::ResponseFuture`, and `server::SendResponse` now all have `stream_id` methods which return the stream ID of the corresponding stream. Signed-off-by: Eliza Weisman <eliza@buoyant.io>

Signed-off-by: Eliza Weisman <eliza@buoyant.io>

hawkw · 2018-07-09T18:21:41Z

src/frame/stream_id.rs

    pub const ZERO: StreamId = StreamId(0);

+    /// The maximum allowed stream ID.
    pub const MAX: StreamId = StreamId(u32::MAX >> 1);

    /// Parse the stream ID


I'm not sure if this function should be publicly exposed or not (perhaps it should be pub(crate) instead?)

hawkw · 2018-07-09T18:22:41Z

src/frame/stream_id.rs

    pub fn is_zero(&self) -> bool {
        self.0 == 0
    }

+    /// Returns the next stream ID initiated by the same peer as this stream
+    /// ID, or an error if incrementing this stream ID would overflow the
+    /// maximum.
    pub fn next_id(&self) -> Result<StreamId, StreamIdOverflow> {


Again, I'm not sure whether or not we want users of the library to be able to construct their own StreamIds. It seems harmless but I'm not sure if there's any use-case for it. Perhaps this should be pub(crate)?

hawkw · 2018-07-09T18:23:59Z

src/proto/streams/streams.rs

+
+    pub fn stream_id(&self) -> StreamId {
+        self.inner.lock()
+            .unwrap()


I thought unwrapping this was probably reasonable, rather than returning a Result, since if the mutex on the stream store is poisoned, we're in a pretty bad state (and the other functions on StreamRef also unwrap).

seanmonstar · 2018-07-09T19:11:10Z

Yea, I'm also a little uncertain about exposing the StreamId type directly... Would it be more conservative to simply expose a u32 instead?

hawkw · 2018-07-09T20:11:50Z

@seanmonstar

Yea, I'm also a little uncertain about exposing the StreamId type directly... Would it be more conservative to simply expose a u32 instead?

I'm unsure...I think the newtype better expresses intent, and the is_client_initiated/is_server_initiated functions might potentially be useful. Personally, I think if we're concerned about exposing this type, restricting the constructors to pub(crate) is the right thing to do, but I could be convinced otherwise.

carllerche · 2018-07-09T20:19:07Z

@seanmonstar Exposing a new type would be more conservative than u32. It could be another StreamId type that doesn't have any additional fns on it?

seanmonstar · 2018-07-09T20:22:39Z

True, a public StreamId that isn't the same as frame::StreamId. Then, we don't have to worry about any traits that are implemented for frame::StreamId, and down the line, its backwards compatible to just change to directly exporting frame::StreamId.

Signed-off-by: Eliza Weisman <eliza@buoyant.io>

hawkw · 2018-07-09T22:14:01Z

@seanmonstar WDYT of 557435c?

seanmonstar · 2018-07-09T22:27:44Z

Looks good to me, if we also put pub(crate) on the internal one, to save us from any accidental exposure.

hawkw · 2018-07-09T22:52:04Z

@seanmonstar

Looks good to me, if we also put pub(crate) on the internal one, to save us from any accidental exposure.

Will do. Note that making frame::StreamId pub(crate) means that we also have to change all pub functions taking one (including those which are in private modules and thus not exported) explicitly pub(crate) as well, to placate the compiler:

error[E0446]: private type `frame::stream_id::StreamId` in public interface
  --> src/frame/data.rs:28:5
   |
28 | /     pub fn new(stream_id: StreamId, payload: T) -> Self {
29 | |         assert!(!stream_id.is_zero());
30 | |
31 | |         Data {
...  |
36 | |         }
37 | |     }
   | |_____^ can't leak private type

...and so on.

hawkw · 2018-07-09T22:55:07Z

Actually, nevermind; the fact that a lot of these types are publically-exposed if the unstable feature is set complicates this a bit. If we want the internal frame::StreamId to be pub(crate), then we'll have to have separate #[cfg("unstable")] pub versions of pretty much every function that takes a frame::StreamId. I'm not sure whether or not having it be pub(crate) is worth that, WDYT?

seanmonstar · 2018-07-09T23:20:19Z

Hmph. I'm not even particularly sure what the unstable feature is really for, so I'll defer the final decision to @carllerche.

hawkw · 2018-07-09T23:40:23Z

@seanmonstar

I'm not even particularly sure what the unstable feature is really for

I believe it's mainly used so that the test support crate can access things that would otherwise not be public.

carllerche · 2018-07-10T15:35:16Z

Yes, it is for the test stuff.

That being said, with the introduction of tests in a sub crate, there is most likely a way to get rid of that feature flag.

hawkw · 2018-07-10T16:57:44Z

That being said, with the introduction of tests in a sub crate, there is most likely a way to get rid of that feature flag.

Okay. That seems out of scope for this branch, though?

carllerche · 2018-07-10T17:02:37Z

yes

carllerche · 2018-07-11T18:35:20Z

To comment on this issue, I'm OK exposing the stream ID where appropriate, but it should be a new type StreamId that has as few fns on it as possible.

hawkw · 2018-07-11T19:40:03Z

@carllerche

To comment on this issue, I'm OK exposing the stream ID where appropriate, but it should be a new type StreamId that has as few fns on it as possible.

That's what this branch does as of commit 557435c --- that commit adds a separate share::StreamId type for externally exposing stream IDs, distinct from frame::StreamId.

The conversation @seanmonstar and I were having was about the fact that the frame::StreamId type (the internal StreamId type) is currently declared as pub (and was before I opened this branch), but the module it's declared in is usually not exported, unless the unstable feature is set. @seanmonstar requested that it be declared as pub(crate) instead so that it's not accidentally exposed, but doing so would add a lot of additional complexity if we still wanted that type to be publicly exposed when the feature flag is set.

carllerche · 2018-07-11T20:23:03Z

src/share.rs

+/// new stream.
+///
+/// [Section 5.1.1]: https://tools.ietf.org/html/rfc7540#section-5.1.1
+#[derive(Debug, Copy, Clone, Eq, PartialEq, Ord, PartialOrd, Hash)]


I would recommend removing Copy. I'd also remove Ord and PartialOrd unless there is a case to keep them.

The issue prompting this change (#289) states that:

I'd expect h2::StreamId to implement at least copy and comparison traits.

I'm not sure whether or not @olix0r has any specific rationale or use-case behind implementing those traits, though?

Clone would make more sense. Copy is pretty strong and adds a forwards compat hazard.

hawkw · 2018-07-12T22:39:46Z

Build failure for 2d63ed4 was due to a connection issue downloading h2spec, so I've restarted it.

This branch adds an accessor for the stream ID to `RecvBody`. It also updates the h2 dependency to v0.1.11, as it requires hyperium/h2#292. Signed-off-by: Eliza Weisman <eliza@buoyant.io>

hawkw added 3 commits July 9, 2018 10:39

Make StreamId pub, add docs

fb9f188

Add stream_id accessors to StreamRef and OpaqueStreamRef

2bbae06

This will be necessary to add similar accessors to the public API types that wrap these. Signed-off-by: Eliza Weisman <eliza@buoyant.io>

hawkw added the enhancement label Jul 9, 2018

hawkw self-assigned this Jul 9, 2018

hawkw requested review from carllerche, seanmonstar and olix0r July 9, 2018 18:14

Add missing doc comments to StreamId

11f0602

Signed-off-by: Eliza Weisman <eliza@buoyant.io>

hawkw changed the title ~~Eliza/expose stream~~ Add stream_id accessors to public API types Jul 9, 2018

hawkw commented Jul 9, 2018

View reviewed changes

Separate public and private StreamId types

557435c

Signed-off-by: Eliza Weisman <eliza@buoyant.io>

hawkw mentioned this pull request Jul 9, 2018

Add stream_id accessor to RecvBody tower-rs/tower-h2#35

Merged

carllerche reviewed Jul 11, 2018

View reviewed changes

Review feedback: derive fewer traits for public API StreamId

2d63ed4

carllerche approved these changes Jul 13, 2018

View reviewed changes

hawkw merged commit f3806d5 into master Jul 13, 2018

carllerche deleted the eliza/expose-stream-id branch October 16, 2018 21:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `stream_id` accessors to public API types #292

Add `stream_id` accessors to public API types #292

hawkw commented Jul 9, 2018

hawkw Jul 9, 2018

hawkw Jul 9, 2018

hawkw Jul 9, 2018

seanmonstar commented Jul 9, 2018

hawkw commented Jul 9, 2018

carllerche commented Jul 9, 2018

seanmonstar commented Jul 9, 2018

hawkw commented Jul 9, 2018

seanmonstar commented Jul 9, 2018

hawkw commented Jul 9, 2018

hawkw commented Jul 9, 2018

seanmonstar commented Jul 9, 2018

hawkw commented Jul 9, 2018

carllerche commented Jul 10, 2018

hawkw commented Jul 10, 2018

carllerche commented Jul 10, 2018

carllerche commented Jul 11, 2018

hawkw commented Jul 11, 2018

carllerche Jul 11, 2018

hawkw Jul 11, 2018

carllerche Jul 11, 2018

hawkw commented Jul 12, 2018

Add stream_id accessors to public API types #292

Add stream_id accessors to public API types #292

Conversation

hawkw commented Jul 9, 2018

hawkw Jul 9, 2018

Choose a reason for hiding this comment

hawkw Jul 9, 2018

Choose a reason for hiding this comment

hawkw Jul 9, 2018

Choose a reason for hiding this comment

seanmonstar commented Jul 9, 2018

hawkw commented Jul 9, 2018

carllerche commented Jul 9, 2018

seanmonstar commented Jul 9, 2018

hawkw commented Jul 9, 2018

seanmonstar commented Jul 9, 2018

hawkw commented Jul 9, 2018

hawkw commented Jul 9, 2018

seanmonstar commented Jul 9, 2018

hawkw commented Jul 9, 2018

carllerche commented Jul 10, 2018

hawkw commented Jul 10, 2018

carllerche commented Jul 10, 2018

carllerche commented Jul 11, 2018

hawkw commented Jul 11, 2018

carllerche Jul 11, 2018

Choose a reason for hiding this comment

hawkw Jul 11, 2018

Choose a reason for hiding this comment

carllerche Jul 11, 2018

Choose a reason for hiding this comment

hawkw commented Jul 12, 2018

Add `stream_id` accessors to public API types #292

Add `stream_id` accessors to public API types #292