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

Adding a set of Event Hooks for a Request #155

Open
seanmonstar opened this issue Jun 28, 2017 · 16 comments
Open

Adding a set of Event Hooks for a Request #155

seanmonstar opened this issue Jun 28, 2017 · 16 comments
Labels
B-rfc Blocked: Request for comments. More discussion would help move this along.

Comments

@seanmonstar
Copy link
Owner

There is desire sometimes to extend the functionality of reqwest at certain points in the request lifecycle, and having event hooks may a good solution to do so. For instance, #87 wants to be able to add some custom logging at different events, and something like NTLM could be supported be exposing some connect event.

We could use a trait, like Events, with default method implementations of doing nothing. That would allow adding new methods without breaking existing code.

An example:

struct Logger;

impl reqwest::Events for Logger {
    fn on_frobnicate(&self, frob: &Frobnicator) {
        info!("frobnicated, {}", frob.details());
    }
}

client.get("https://hyper.rs")
    .events(Logger);
    .send()?;

Some questions that need resolving:

  • Should this be added on the Client, and the same instance used for all the requests and responses, or should they be added on a per Request basis?
  • What events are useful, and what arguments should they receive? Care should be taken that we don't expose implementation details that would prevent us from changing internal things.
@lucab
Copy link
Contributor

lucab commented Jul 13, 2017

I think I have similar usecase here (adding custom auth headers to a request built internally in rust-tuf library), which I think would fit in this Event Hooks design if you allow the hook to mutate the request itself.

Providing some partial datapoints to your questions:

  • those hooks are set on the Client, and they internally have some logic to apply different actions to specific requests
  • pre-send event in my case, and enough arguments to introspect&mutate a request before sending it (common tasks would be setting headers and adding query parameters to url)

@theduke theduke mentioned this issue Jul 13, 2017
@theduke
Copy link
Contributor

theduke commented Jul 13, 2017

I think one should definitely be able to set hooks on the whole client.
Overwriting/setting on the request builder could be an additional option.o

Hooks I definitely want:

  • pre_send(&self, &mut Request)

  • on_result(&self, &Request, &Result<Response>)

Or alternatively:

  • on_ok(&self, &Request, &Response)
  • on_err(&self, &Request, &Error)

The Request is probably being consumed while sending right now?
I'd be able to live with the on_* hooks only receiving the Result/Response/Error, but that's a bit limiting.

For example, in debug mode I might want to print out the request url, headers and body along with the response only if the request failed.

The handler would either have to be Sync to allow &self, or implement Clone and be cloned for each request. I guess the right way to go here is to require Sync.

@echochamber
Copy link
Contributor

Just want to make sure I understand, hooking into the process would just be for "read only" reasons right?

Exposing a hook which would allow a listener to edit the request object on the pre_send. That seems like it could be abused for some really surprising behavior.

@seanmonstar
Copy link
Owner Author

I don't know if it should be readonly or get mutable access. I don't have many use cases myself, but collecting them here would help determine what is needed or useful.

@theduke
Copy link
Contributor

theduke commented Jul 15, 2017

My primary use case would be to add authorization headers.

@seanmonstar seanmonstar added the B-rfc Blocked: Request for comments. More discussion would help move this along. label Aug 19, 2017
@KodrAus
Copy link
Contributor

KodrAus commented Sep 26, 2017

I like the idea of being able to log various happenings in reqwest that I couldn't necessarily reach otherwise. How would we expect this feature to interact with other features that span multiple requests like redirects?

I also agree with @echochamber with regards to mutability. I think it's really nice and convenient to be able to register hooks that manage cross-cutting concerns for all requests, but when you lose locality of mutability to what's being mutated then it becomes harder to reason about what's going on.

@leoschwarz
Copy link

Maybe mutable interjection in the request is a feature orthogonal to the fine grained logging/status reporting requested here, the later could be added at most places without increasing complexity too much. The mutability would serve more to provide a means to implement something like a "middleware" ("interceptor"/"rewriter" or whatever to call it) rewriting requests and responses (so it needs two access points). In that case maybe these concerns belong to a different issue?

The usefulness of such a middleware API would be that the alternative of wrapping the client and handling stuff like auth or mocking (#154) is not composable, so everyone has to write it once by hand, while with middlewares could be reused between crates. Downside is that it's probably rather hard to make a good API, because for example for mocking requests completely I'd like to be able to return a success response without any HTTP request having made at all.

@mattgathu
Copy link

What events are useful, and what arguments should they receive?

At the moment, it would be useful for me to know:

  • when the client connects via a http proxy. (when the connection to the proxy server is established).
  • when a target url has been resolved and the target ip address is known.

@mattias-p
Copy link
Contributor

I'd like to have on_redirect(&self, &Result, &Response). I'd use it to log the value of the Location header. This kind of overlaps with #281. If both are implemented I'd only use one of them.

@ticky
Copy link

ticky commented Aug 31, 2018

Aside from mocking which would definitely be useful, a generic API for intercepting and optionally interrupting and responding to a request would be useful for implementing, for example, a caching plugin.

@arcrose
Copy link

arcrose commented Sep 26, 2018

People's points about generic request interception and modification not being reqwest's responsibility make a lot of sense, and I find them pretty agreeable. Adding specific handlers just for a couple of cases (even to hyper rather than reqwest) might be a better fit. My personal want for a feature like this is to be able to do some work in between the establishing of a TLS connection (i.e. playing with the CONNECT request).

@ticky
Copy link

ticky commented Sep 26, 2018

I’d be okay with this being Hyper’s responsibility, but we’d then require some way to gain access to the underlying Hyper instance, which I don’t believe reqwest currently exposes any such hooks.

@danieleades
Copy link
Contributor

how do we feel about the middleware approach that Surf has used?

@stone-bits
Copy link

how do we feel about the middleware approach that Surf has used?

That's what I was looking for! Thanks!

@sylvain101010
Copy link

Hi,
I would also love to see this implemented.
My use case is http tracing and debugging. I think Go has a really great interface for this: https://blog.golang.org/http-tracing which can be a great source of inspiration.

@DCjanus
Copy link

DCjanus commented Feb 17, 2021

Maybe middleware or interceptor would be more convenient to implement, and with middleware support, features like ratelimittracing or simple log would be able to impl by my self.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
B-rfc Blocked: Request for comments. More discussion would help move this along.
Projects
None yet
Development

No branches or pull requests