lostisland / faraday

Simple, but flexible HTTP client library, with support for multiple backends.
https://lostisland.github.io/faraday
MIT License
5.74k stars 976 forks source link

Streaming Stop with Connection Close? #1501

Closed fearlessfrog closed 1 year ago

fearlessfrog commented 1 year ago

Basic Info

Issue description

Using connection close does not stop the req.options.on_data streaming proc callbacks for a stream.

Another way to ask this one would be what would be the best way to stop/cancel a stream that is already running given access to the Faraday connection, env and req/resp objects?

Steps to reproduce

  1. Set up conn.post(uri(path) do |req| req.options.on_data call
  2. In proc callback call conn.close
  3. The req.options.on_data proc is still called as data streams in.

Thanks for any help/advice.

iMacTia commented 1 year ago

Hi @fearlessfrog, and thank you for opening this issue.

I can see why you've tried with conn.close, but unfortunately that method's implementation is up to middleware/adapters and Faraday is simply passing the call along. That's also a method defined at the connection level, while I'd argue what you're really trying to do is to cancel the request.

If the logic behind this decision is known ahead of time (e.g. based on a chunk received), you should be able to raise an exception inside the on_data block to stop execution. Although I agree this is far from ideal, I wasn't able to find a proper way to do this while looking at the Net::HTTP doc.

If you're trying to implement something relying on an external event (e.g user clicking on a cancel button), then that's a bit harder, but the only way you could be able to cancel a streaming request from the outside is if the request was running asynchronously. This would technically be possible with some shared memory flag, but I'd need additional details on the implementation in order to advice further.

fearlessfrog commented 1 year ago

Thanks for taking a look. A custom exception raised in the on_data block does do the trick and works when caught by the calling job. The stop request is via a user cancel action, and I already use Redis etc to flag that within the running asynch bg jobs; it looks for that state as the on_data proc executes and raises the exception as needed. I just wasn't sure if there was a better way or something I missed or not. I do pass the request object to the proc (using ruby-openai with the new chat streaming for this), so in the future it could call a new 'close' or 'stream_close' on that, but I'm not familiar enough with Faraday to know if that fits the overall design. Thanks again, and happy for this to be closed if wanted.

iMacTia commented 1 year ago

Thank you for reporting back @fearlessfrog! I'm glad to hear that worked, and appreciate raising exceptions doesn't feel "right".

Unfortunately, Faraday does not manage the underlying connection and is therefore unable to close the connection without this functionality being exposed by the underlying adapter(s). I was unable to find anything like that for Net::HTTP, and I suspect this is mostly due Ruby's non-concurrent nature, which makes these things feel less necessary.

I'm gonna close the issue considering you were able to achieve what you wanted, but if anyone more knowledgeable would like to contribute in making this nicer I'd happily review a new issue/PR ❤️