aspotify icon indicating copy to clipboard operation
aspotify copied to clipboard

Wrap reqwest client in Arc

Open oleggtro opened this issue 3 years ago • 4 comments

as discussed in #12 I propose wrapping the reqwests client in an Arc to make in clonable

oleggtro avatar Oct 03 '21 13:10 oleggtro

when merging, can u please label this pr with hacktoberfest-accepted ?

oleggtro avatar Oct 03 '21 13:10 oleggtro

Oh no, that's not what I meant 😅. Reqwest's Client is already internally ref-counted and cheaply clonable, so it's never needed to Arc it - I was proposing to internally Arc the entire aspotify::Client, so that ClientCredentials gets Arc-ed (since that's the thing that is expensive to clone).

when merging, can u please label this pr with hacktoberfest-accepted?

Sure! Should I just create a GitHub label then?

SabrinaJewson avatar Oct 03 '21 14:10 SabrinaJewson

So you want to return an Arc-ed Client from building it?

Sure! Should I just create a GitHub label then?

That would be perfect! thx

oleggtro avatar Oct 03 '21 14:10 oleggtro

So you want to return an Arc-ed Client from building it?

No, the idea is to do what Reqwest does: have Client internally just contain an Arc<ClientInner>. ClientInner is private and contains all the fields that Client currently does. That way cloning a Client is just a matter of incrementing the refcount of the ClientInner.

If new would return a pre-made Arc<Client>, we might as well just tell users to make Arc<Client> themselves - the only benefit of this change would be the convenience of not having to import Arc.

SabrinaJewson avatar Oct 03 '21 15:10 SabrinaJewson