hyperium / http

Rust HTTP types
Apache License 2.0
1.14k stars 284 forks source link

Can't iterate `http::header::ValueDrain` non-destructively #531

Open lilyball opened 2 years ago

lilyball commented 2 years ago

I need to remove an entry from a HeaderMap and then iterate the removed values non-destructively. The reason for this is I need to mutate the HeaderMap while I'm processing the removed values, but I need to process the removed values as &[u8] or &str. Doing anything else will involve reallocating for every value (I need to trim off the front) and that should be unnecessary for what I'm doing. If HeaderValue exposed its inner Bytes representation then I could use that (as that is cheaply cloneable), but it doesn't, and so I can't.

The simplest solution here would be if http::header::ValueDrain supported iterating references to its values. I know it's already an iterator, but it could have an .iter(&self) method that returns an iterator that borrows the drain (and then &'_ ValueDrain could also implement IntoIterator to return that same borrowed iterator). This way I can work with references to the values, or iterate the values multiple times, without having to collect them into a separate allocation (such as a Vec).

lilyball commented 2 years ago

Argh, I just realized that this doesn't work anyway for what I'm actually trying to do. I'm actually fetching a few elements off of the drain before collecting the rest into a Vec because I need to drop the drain before mutating the map and this is an optimization to avoid the Vec allocation if the drain is small.

But this works right now for small drains without allocating because I'm getting a HeaderValue out. However if I'm working with references then I can't get an owned value out of it and would need to allocate anyway.

So basically, this doesn't actually work for what I want it to do, but it would still be nice just from the perspective of "I want to iterate the drain a few times" or alternatively "I want to work with references and I don't care that the HeaderMap is still borrowed".

lilyball commented 2 years ago

Really what I need to avoid allocations here is a way to convert the HeaderValue into its underlying Bytes, but since that seems to be intentionally hidden I can't do that. If it had some sort of.into_maybe_shared<T>(self) -> T where T: FromIterator<u8> + 'static method as the dual to .from_maybe_shared() that would be usable though it feels a little awkward.

lilyball commented 2 years ago

This is especially frustrating because it turns out that ValueDrain today is actually needlessly borrowing the HeaderMap as it actually moves everything into a fresh Vec immediately. I'm assuming it's doing this with the intention of later actually draining values from the map on demand without the extra allocation, but as it stands, it means I'm getting an allocation where I shouldn't need one, and then I need a second duplicate allocation just to drop that borrow.

lilyball commented 2 years ago

Thinking about this again fresh, the OccupiedEntry itself is capable of nondestructively iterating. Since the ValueDrain still holds a borrow on the map, it's not particularly useful for my current purposes to try to nondestructively iterate the drain (though I still hold that it has potential utility in generic code). And since the drain allocates a vec internally instead of draining elements in-place, the best thing I can do right now is actually to just iterate the entry and clone each value into a new Vec myself, before then asking the entry to just remove itself. This avoids the intermediate hidden Vec allocation, at the cost of extra atomic operations (the HeaderValue has to bump retain counts on its inner Bytes on the clone).

I am not a fan of baking in assumptions about implementation details here, and I wish I could just move the values into my Vec, but the drain's hidden inner Vec makes that wasteful. Another option might be to swap every HeaderValue with a HeaderValue::from_static("") as that avoids even having to bump retain counts, it's just a bit weirder.

I realize I've basically been ranting in this issue, and I think I'm done. I have a plan now for at least avoiding one Vec allocation, though I wish I didn't have to write non-intuitive code to do so.