Introduce tower_grpc::metadata::MetadataMap type #92

per-gron · 2018-11-08T17:11:39Z

As per the discussion in #90, it wraps http::HeaderMap to abstract away the fact that tower-grpc is HTTP based. At this time this class is a pure wrapper that does not actually implement any of the gRPC metadata specific stuff (such as binary fields), that will come later.

My goal with this PR is to make MetadataMap (largely) equivalent to HeaderMap while not exposing http types.

As per the discussion in tower-rs#90, it wraps http::HeaderMap to abstract away the fact that tower-grpc is HTTP based. At this time this class is a pure wrapper that does not actually implement any of the gRPC metadata specific stuff (such as binary fields), that will come later.

seanmonstar · 2018-11-08T20:19:34Z

Wow! I haven't reviewed yet, I'm about to look through, but first thoughts: you really took this seriously, this is impressive!

seanmonstar

I see now that much of the diff is wrapping the http::header internals. Well done!

What was the decision regarding -bin headers?

seanmonstar · 2018-11-08T20:22:59Z

src/metadata_map.rs

+    /// Convert an HTTP HeaderMap to a MetadataMap
+    pub fn from_headers(mut headers: http::HeaderMap) -> Self {
+        let mut map = Self::with_capacity(headers.len());
+        for (name, values) in headers.drain() {


Is there a reason to move these from the headers into a brand new map? Could this just be MetadataMap { inner: headers }?

http::HeaderMap is equal to http::HeaderMap<http::HeaderValue>. MetadataMap however has a http::HeaderMap<MetadataValue>, so when constructing a MetadataMap the values need to be wrapped individually.

MetadataMap can't just wrap a http::HeaderMap<http::HeaderValue> because there are some important methods that return references to those values, which would be unimplementable then (or would require a separate MetadataValueRef type that wrapped HeaderName&).

(There is a similar issue for methods that return references to map keys but I worked around that by returning &strs instead, which is basically what HeaderName is anyways after construction and you don't need to quickly compare them with standard HTTP header names.)

At least this code just moves all the headers, so runtime is proportional to header count but not size.

We could still do it, though it would require an unsafe transmute. stdlib relies on this being valid in a number of places. For example, Path::new where Path(OsStr):

pub fn new<S: AsRef<OsStr> + ?Sized>(s: &S) -> &Path { unsafe { &*(s.as_ref() as *const OsStr as *const Path) } }

I would use this strategy personally. I think a zero-cost MetadataMap wrapper is critical here, otherwise switching is not be worth it.

Cool idea! Done.

seanmonstar · 2018-11-08T20:24:05Z

src/metadata_map.rs

+    }
+
+    /// Convert an HTTP HeaderMap to a MetadataMap
+    pub fn from_headers(mut headers: http::HeaderMap) -> Self {


Wasn't the point to hide any http types? Why did you feel this constructor was needed?

These two conversion methods are used in src/request.rs. Perhaps I should make them pub(crate) instead.

I mean... it doesn't seem terrible to have conversions. Being able to easily convert between the two does not violate "hiding".

Ok, I'll keep them public then.

per-gron · 2018-11-09T13:20:53Z

What was the decision regarding -bin headers?

This PR does not address that issue. This just hides the HeaderMap type from tower-grpc users, while exposing an API that is almost identical. I intend to address -bin headers in a later PR. I felt this PR was already large enough 😃

Differences include:

Wherever HeaderMap methods return a HeaderName&, the corresponding MetadataMap method returns str& because there is no MetadataKey value stored anywhere to take a reference of.
There is no Index operator, because I plan to make separate getters for binary and non-binary fields and then it's not clear if indexing should get binary or non-binary fields. Also I'm not a big fan of having such a convenient shorthand for a panic-ing operator.

carllerche

Thanks for doing this 👍 Looking great.

Some thoughts?

Could you add some some top level docs (even just a few lines) for the metadata module.
To avoid cluttering the metadata module, maybe we can create metadata::errors and export all error types there. Those are less commonly needed and it would make exploring mod metadata easier.

Additional inline comments.

src/metadata_key.rs

carllerche · 2018-11-16T19:17:17Z

src/metadata_map.rs

+    }
+
+    /// Convert an HTTP HeaderMap to a MetadataMap
+    pub fn from_headers(mut headers: http::HeaderMap) -> Self {


I mean... it doesn't seem terrible to have conversions. Being able to easily convert between the two does not violate "hiding".

carllerche · 2018-11-16T19:32:35Z

src/metadata_map.rs

+    /// Convert an HTTP HeaderMap to a MetadataMap
+    pub fn from_headers(mut headers: http::HeaderMap) -> Self {
+        let mut map = Self::with_capacity(headers.len());
+        for (name, values) in headers.drain() {


We could still do it, though it would require an unsafe transmute. stdlib relies on this being valid in a number of places. For example, Path::new where Path(OsStr):

pub fn new<S: AsRef<OsStr> + ?Sized>(s: &S) -> &Path { unsafe { &*(s.as_ref() as *const OsStr as *const Path) } }

I would use this strategy personally. I think a zero-cost MetadataMap wrapper is critical here, otherwise switching is not be worth it.

src/metadata_map.rs

…ster

This makes the API more self-symmetrical and more similar to HeaderMap.

carllerche

LGTM modulo that commented out bit that depends on hyperium/http#279.

@seanmonstar you good with this?

carllerche · 2018-11-21T21:38:23Z

@per-gron I pushed an http release: https://crates.io/crates/http/0.1.14

seanmonstar · 2018-11-22T02:32:34Z

src/metadata_value.rs

+
+    #[inline]
+    pub(crate) fn from_header_value(header_value: &HeaderValue) -> &Self {
+        unsafe { &*(header_value as *const HeaderValue as *const Self) }


I believe the only way this isn't UB is if MetadataValue has the #[repr(transparent)] attribute on it. Otherwise, struct layout is undefined.

As long as the struct has a single field and has no other repr on it, it is pretty much guaranteed to have the same layout. I don’t think it has been expressly stated, but std assumes this and so do many other crates.

This technique is basically the only way to define your own DST and has been shown to me by rust team members.

I think we can rely on it.

@seanmonstar thanks for the link to #[repr(transparent)]! I was not aware of it. It seems like the perfect thing for this use case, as that is exactly the guarantee that is necessary in this case. I have updated the PR to use it.

I did it realize transparent was stable... that is the best option.

per-gron · 2018-11-22T20:59:10Z

I fixed the last issues. I think this is ready to merge now.

per-gron · 2018-12-05T10:08:33Z

I'll go ahead and merge this now.

per-gron added 2 commits October 30, 2018 15:49

Rename Request::headers to Request::metadata

7ff6b77

per-gron mentioned this pull request Nov 8, 2018

Make HTTP a hidden implementation detail, don't expose it in the public API #90

Closed

seanmonstar reviewed Nov 8, 2018

View reviewed changes

Merge remote-tracking branch 'origin/master' into request-metadata

480d4f1

carllerche suggested changes Nov 16, 2018

View reviewed changes

per-gron added 5 commits November 20, 2018 12:50

Address some review comments

aab5ea4

Use unsafe transmutes to make MetadataMap <=> HeaderMap conversion fa…

4f93350

…ster

Address review comments

b493f5d

Make the interop tests build

4119ce9

Say why some code is commented out

ecce2a5

per-gron force-pushed the request-metadata branch from 4a9e79b to 3437f90 Compare November 20, 2018 13:22

Use unsafe transmutes for returning &MetadataKey from MetadataMap

e48818f

This makes the API more self-symmetrical and more similar to HeaderMap.

per-gron force-pushed the request-metadata branch from 3437f90 to e48818f Compare November 20, 2018 13:36

carllerche approved these changes Nov 21, 2018

View reviewed changes

seanmonstar reviewed Nov 22, 2018

View reviewed changes

per-gron added 2 commits November 22, 2018 12:18

Use newly exported IterMut and ValuesMut from http to resolve TODOs

4f9bab2

Add #[repr(transparent)] annotations to types that rely on that behavior

ba6ff3b

This was referenced Nov 25, 2018

Expose extensions map in Request #95

Closed

Expose extensions map in Request #94

Closed

per-gron merged commit ca2ee54 into tower-rs:master Dec 5, 2018

per-gron deleted the request-metadata branch December 5, 2018 10:08

per-gron mentioned this pull request Dec 5, 2018

Add support for binary custom metadata fields #102

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce tower_grpc::metadata::MetadataMap type #92

Introduce tower_grpc::metadata::MetadataMap type #92

per-gron commented Nov 8, 2018

seanmonstar commented Nov 8, 2018

seanmonstar left a comment

seanmonstar Nov 8, 2018

per-gron Nov 9, 2018

carllerche Nov 16, 2018

per-gron Nov 20, 2018

seanmonstar Nov 8, 2018

per-gron Nov 9, 2018

carllerche Nov 16, 2018

per-gron Nov 20, 2018

per-gron commented Nov 9, 2018

carllerche left a comment

carllerche Nov 16, 2018

carllerche Nov 16, 2018

carllerche left a comment

carllerche commented Nov 21, 2018

seanmonstar Nov 22, 2018 •

edited

carllerche Nov 22, 2018

per-gron Nov 22, 2018

carllerche Nov 23, 2018

per-gron commented Nov 22, 2018

per-gron commented Dec 5, 2018

Introduce tower_grpc::metadata::MetadataMap type #92

Introduce tower_grpc::metadata::MetadataMap type #92

Conversation

per-gron commented Nov 8, 2018

seanmonstar commented Nov 8, 2018

seanmonstar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

per-gron commented Nov 9, 2018

carllerche left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carllerche left a comment

Choose a reason for hiding this comment

carllerche commented Nov 21, 2018

seanmonstar Nov 22, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

per-gron commented Nov 22, 2018

per-gron commented Dec 5, 2018

seanmonstar Nov 22, 2018 •

edited