Consider making Authenticator a Clone

[nagisa]

In many instances the libraries based on yup_oauth2 will take the Authenticator by value. This, however, prevents reuse of the tokens between different libraries or even instances of whatever type represents some sort of behaviour of a single library.

It seems like the primary reason Authenticator currently cannot be Clone is its implementation of Storage, which internally stores a Mutex<HashMap<_, _>>. A trivial solution would be to wrap that into an Arc, however, using some lock-free thread-safe storage would likely be a more lasting solution.

[ggriffiniii]

I'll chime in with my thought, but leave it up to @dermesser to see if he's interested in pursuing this. I would like to leave it up to the user of the library to choose what they need. I tend to avoid wrapping Arc's in the internal implementation unless the implementation requires the cloning. For one it's not obvious without looking at the implementation how efficient the .clone() method will be and two not every user requires cloning so providing it makes every user pay an upfront cost (albeit a tiny and inconsequential one for most users) whether they need the functionality or not. If it was difficult for users to provide their own Clone implementation then there could be some justification to include that in the implementation as the cost/benefit ratio would be tilted in the benefit direction, but it should be pretty straightforward to wrap Authenticator in an Arc/Rc to provide a clone for users that require it so in my opinion the cost is not worth the benefit.

[dermesser]

I agree with @ggriffiniii although mostly with the last sentence. Given we are dealing with API interactions here, the overall performance impact is usually negligible. But wrapping Authenticator objects in your code seems like the right level of abstraction. Making everything thread-safe would also restrict the kinds of caches we could implement, I believe.

[nagisa]

I would like to leave it up to the user of the library to choose what they need.

Consider probably the most obvious way to write down a crate using Authenticator (presuming Authenticator does not need to be wrapped in Arc):

// in banana_crate
struct Banana<C> { authenticator: Authenticator<C>, ... }

impl<C> Banana<C> {
    fn new(auth: Authenticator<C>, ...) -> ... { ... }
}

And this is, broadly, fine. Until some application depends on both banana_crate and peach_crate, written mostly the same as Banana as far as handling of Authenticator is concerned:

// in peach_crate
struct Peach<C> { authenticator: Arc<Authenticator<C>>, ... }

impl<C> Peach<C> {
    fn new(auth: Authenticator<C>, ...) -> ... { ... }
}

There is nothing user of both of these crates can do then to make both banana_crate::Banana and peach_crate::Peach use the "same" Authenticator "instance" (and the token cache contained therein)…

We could go and recommend every crate accepting Authenticator instances to accept something like Arc<Authenticator> instead, but in that case why not make Authenticator itself Clone and share the caches? Perhaps we could also just stomach the significantly higher cost of re-obtaining tokens multiple times for each of the crate used, but that’s just a disappointing design flaw – what if an application ultimately ends up depending on and actively interacting with 20 authenticator-using crates? 100?

[ggriffiniii]

Why wouldn't the clients accept a shared reference to the Authenticator? Why do they need to take ownership?

[nagisa]

That’s a good question. Historically none of the generated google API bindings took Authenticator by a value:

See e.g. PubSub, which takes anything implementing GetToken which in turn is only implemented for by-value Authentictor (for an old version of yup-oauth2).

As far as users of modern yup-oauth2 are concerned, I could only find 3 crates in the list of reverse dependencies:

• bigquery-storage takes Authenticator by-value;
• libunftp does not expose API that would take Authenticator at all, instead they construct one from path internally;
• hedwig is the only one I can really explain since I developed it – originally took Authenticator<...> and wrapped it into an Arc internally, but now takes Arc<Authenticator<...>> to enable some reuse. The reason its between these two options is:
  • this kind of API felt least inconvenient for the API user to work with;
  • there’s no way to implement a useful async fn that captures an authenticator by reference and then returns a 'static future, which is pretty much a requirement for anything but trivial futures I feel.

[nagisa]

For one it's not obvious without looking at the implementation how efficient the .clone() method

Strongly agreed on this point, however. I recently filled hyperium/hyper#2239 for a similar reason.

[danburkert]

Chiming in here to say that having Authenticator be clone would be very nice - the additional cost is negligible (there is already internal allocation and locking), and it's the norm in the hyper and async ecosystems. It's a huge quality of life improvement to avoid lifetimes when working with async code.

[dermesser]

we can take a look again... some things have changed since this issue was created.

Looking at the code, I believe the main reason making Clone hairy is the Storage part of the authenticator.

(aa) Cloning it is not always trivial, especially with new custom storage providers, but also with existing ones (#146), although I guess it could be done.

(b) Storage is supposed to be shared among all clients. It is not that useful to refresh a token a dozen times just because you have cloned Authenticators all over the place. This also can be made to work, with some layer of indirection.

[ggriffiniii]

I personally have been swayed by the arguments in favor of adding Clone, but again it's up to you @dermesser. Like mentioned above since the authenticator is designed to be shared its already using internal locking and shared references for everything. Throwing all of the Authenticator implementation inside an Arc should be pretty straightforward if we wanted to do it.

[dermesser]

@ggriffiniii I will trust your expertise, then :) If this is only about moving the internals to a separate struct inside an Arc, then I don't see a problem indeed.