Update `extensions` to be cloneable

[LucioFranco]

I propose that we move forward to allow Request<B: Clone> to impl Clone. The big blocker here is that extensions is not clonable. This could be fixed by making a AnyClone trait:

pub trait AnyClone: Any + Clone {}

The downside here is that all things you store in extensions is cloneable but I think this is a fair trade off since mostly its just information that could be thrown behind an arc or bytes.

cc @seanmonstar @carllerche @hawkw @davidbarsky

[dekellum]

Some thoughts from an interested user:

Given new requirement on extensions, a breaking change then?

You would apply the same to Response?

It sounds like you would impl Clone where B: Clone so its kind of optional for bodies, but over time there might be more code expecting this. An aspect that gives me pause is that hyper's default Body implements Stream (and is not Clone yet) and http_body::Body defines a Stream-like extension interface. How should implementations handle cloning a Stream-type after it has been polled? Do cloned Streams start from the beginning (if that's even possible) or from the point they are cloned?

With synchronous corollaries, we would typically not clone an Iterator, but clone the thing that can provide a new iterator, e.g. Vec or any impl IntoIterator. In other words, adding a Clone requirement suggests that maybe, in a breaking release, we should also change to requiring a Body implement IntoStream? (Note unfortunately there is already something called IntoStream, for an entirely different purpose, in futures 0.3.)

With clone in play, the HeaderMaps in Request and Response might benefit from Arc or Cow?

Would the next breaking release also be time to consider integrating http_body into this crate? Or is that considered out of scope? (As an observer I've hoped the second crate was a temporary solution.)

[LucioFranco]

So I think the real goal here should be to make Parts cloneable and then since bodies are generic let bodies implement clone if they can. For streams, it should be possible to detect if the request failed before the stream was extracted to retry it then. The big issue for me is making Request a bit easier to deal with and extensions seem to always be the big blocker here.

[davidbarsky]

I've shared my opinion on extensions previously and it still hasn't changed. Short version: I think should be removed. Long version:

• There are three logical parts of an http::Request:
  • The first are the Parts, which carry the protocol version, method, URI, and headers. These are trivially cloneable relative to the next two. I wouldn't even try to place that data in an Arc, as it might be cheaper to perform a memcpy on smaller bits of data than bumping a refcount on an Arc.
  • Extensions, which is a typemap that might not contain cloneable data. @LucioFranco proposes that all types that are stored in a typemap should implement Clone, but I'm not sure that's the right approach. Instead, I'd like to remove extensions wholesale and replace their use-cases with task and thread locals for handling implicit context. I believe that thread/task locals (especially thread/task locals that propagate to child tasks using structured concurrency) solve the problem that http's extensions try to tackle in a more elegant and general-purpose way. I'm aware of some specialized use-cases where task/thread locals wouldn't be a good fit, but I don't believe costs should be borne by all users of http. Critically, I think cloning an http::Request is a far more common use-case than placing data into extensions.
  • Body: for streaming bodies, it's not at clear how bodies should be cloned in a general-purpose way. We shouldn't even try to handle that here!

[LucioFranco]

@davidbarsky the issue is that task/thread-locals don't work with most channel backed offtask clients like hyper and tower-buffer. While ext do work with this setup. They couple data that is specific to the user within the request.

or streaming bodies, it's not at clear how bodies should be cloned in a general-purpose way. We shouldn't even try to handle that here!

Agreed, but there are special cases like gRPC where we have total control over this and could probably pull of some sneak stuff related to this. Having parts be clonable would be a huge help here.

[LucioFranco]

I wouldn't even try to place that data in an Arc, as it might be cheaper to perform a memcpy on smaller bits of data than bumping a refcount on an Arc.

I mean its your choice how to implement clone, if memcpy is cheap then do that if arc bump is cheap then do that?

[davidbarsky]

The issue is that task/thread-locals don't work with most channel backed offtask clients like hyper and tower-buffer. While ext do work with this setup. They couple data that is specific to the user within the request.

Sure. In those cases, I think the appropriate course of action is to implement an extension trait that performs a tracing-subscriber-style lookup for the request context. That being said, I believe that most uses of extensions are used to change control flow prior to the request reaching hyper::Client or tower-buffer.

Agreed, but there are special cases like gRPC where we have total control over this and could probably pull of some sneak stuff related to this. Having parts be clonable would be a huge help here.

My argument boils down that we don't need to settle for just making Parts cloneable; we can make the entire http::Request cloneable by moving extensions out of http::Request.

[hawkw]

I'm not convinced that task locals are an acceptable substitute for extensions. Linkerd makes makes use of extensions in a lot of places, and although I'll have to take a closer look, I think there are a lot of use-cases that can't be replaced with task-local storage.

FWIW, when Linkerd's retry code "clones" requests, we just clone the parts + body, and create new extensions. A lot of the data we store in extensions is specific to an individual instance of a request — e.g. we want to record latencies for retries separately.

[LucioFranco]

@hawkw that seems like it could easily be done in a clone where you basically clear the data 👍

[zuoxinyu]

@hawkw that seems like it could easily be done in a clone where you basically clear the data 👍

Agreed, maybe roughly implement like it does in http-types.