Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Where to store state in reqwest-middleware? #45

Open
mre opened this issue Jun 17, 2022 · 6 comments
Open

Where to store state in reqwest-middleware? #45

mre opened this issue Jun 17, 2022 · 6 comments
Labels
documentation Improvements or additions to documentation reqwest-middleware Issues related to the core middleware lib

Comments

@mre
Copy link

mre commented Jun 17, 2022

I tried to add a new middleware for handling rate-limit headers. The idea was to use https://github.com/mre/rate-limits for extracting these headers and sleeping for a certain duration if we got rate-limited.

Even in the case of getting rate-limited, we can still return the response immediately. Only on the next request would we have to sleep until the limit got lifted.
The way I thought I could implement it was to use a HashMap<String, Duration> from which I would look up the wait time for the given host. However it looks like the Middleware trait uses a non-mutable &self parameter, so I can't update the HashMap field stored in self unless I'm missing something.

The other alternative I considered was to store the HashMap inside Extensions, but that turned out to be quite unergonomic.

Am I missing anything obvious? Would love to get some feedback.

@mre mre added the bug Something isn't working label Jun 17, 2022
@mre
Copy link
Author

mre commented Jun 17, 2022

Note: the bug label was added automatically. The issue is more of a conversation starter, so neither bug nor feature sounded right when I picked the issue template. Github Discussions might work well for such kind of issues in the future. 😉

@tl-rodrigo-gryzinski
Copy link
Contributor

tl-rodrigo-gryzinski commented Jul 21, 2022

You're not missing anything, there's no mechanism for mutable state in the middleware data itself, since we keep the middleware stack inside Arcs we can't have a handle method that takes mutable references.

Using Extensions would work though that was intended for passing data across different middleware instances in the stack. Could wrapping the map in a Mutex or using dashmap work for your use case? Should be more ergonomic than extensions

@conradludgate conradludgate added documentation Improvements or additions to documentation and removed bug Something isn't working labels Aug 30, 2022
@conradludgate
Copy link
Collaborator

Hi, @mre. Did you manage to solve your issue using extensions/interior mutability? I've re-labeled this issue as documentation because it would be good to have some examples of using a Mutex to update some shared state.

@conradludgate conradludgate added the reqwest-middleware Issues related to the core middleware lib label Aug 30, 2022
@mre
Copy link
Author

mre commented Sep 27, 2022

Sorry, didn't find the time to look into it yet. 😕

@DerMolly
Copy link

I'm currently stuck at the exact same point.

@tl-rodrigo-gryzinski said:

Using Extensions would work though that was intended for passing data across different middleware instances in the stack.

At least in my testing the extensions seem to be cleared between calls. Maybe you juts get a different extension each time. So this could not work ever or am I missing something?

If not would it be possible to have some extra state object passed into the handle function or something similar?

@conradludgate
Copy link
Collaborator

conradludgate commented Jan 1, 2023

The issue is that in theory you could be making a different request on many threads all at once. Any state will therefore be backed by a mutex or another concurrent structure.

I see no sensible way for us to use a mutex within the scope of reqwest-middleware for you, since they will be longer lived than your middleware can do - which is an optimisation that really matters. (ie, I'm not comfortable having us being limited to 1 request at a time, or even 1 of each middleware process since so many are stateless),

You can make a custom middleware with the state stored inside a Mutex/RwLock, or use some other concurrent data structures

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation reqwest-middleware Issues related to the core middleware lib
Development

No branches or pull requests

4 participants