search

davidMcneil / rants

An async NATS client library for the Rust programming language.
Apache License 2.0
81 stars 11 forks source link

RFC: Refactor request to support timeouts and always clean up on Drop #25

Closed film42 closed 4 years ago

film42 commented 4 years ago

It might actually be worth splitting this PR into two.

This PR adds:

  1. a new request_with_timeout interface that allows the caller to supply a duration. The existing request method will call into the new timeout method with None and the impl will not apply a timeout. No breaking changes.
  2. a new Request type that holds the logic and ownership of the reply_to subscription so it can remove it from the client when the request is dropped. So whether a timeout happens, subject parsing error, or the caller gives up, ... doesn't matter, the reply_to will always be removed from the inbox mapping.

The Request type needs some improvement, but it does solve https://github.com/davidMcneil/rants/issues/4. I don't like the way drop is implemented, but it does seem reasonable. I'm not sure how much slower it will be since the lock could be expensive if the client is busy with a line of lock acquirers. I also think it might be worth setting a flag on error so we only attempt to cleanup if we absolutely need to.

I also don't like how the Request type is very involved with spawning the clients request handler. Not pretty at all. Still, I was hoping for some feedback :+1: / :-1: on the approach, and if :-1: , what I should do differently.

davidMcneil commented 4 years ago

I know this does not directly relate to this PR, but this comment can be removed right? After your work in #3 .

film42 commented 4 years ago

I know this does not directly relate to this PR, but this comment can be removed right?

3 doesn't actually change the connect behavior. We only make the round-trip wildcard request mapping subscription at the first call (just in time) to Client.request.

davidMcneil commented 4 years ago

Thanks!