HTTP1 stream

[hissssst]

This is a request for comments about initial HTTP1 streaming implementation from ideas in #236

[josevalim]

Btw, there may actually be a simpler implementation here.

The main insight here is to have one process hold the connection and the other do the streaming.

You can create any stream by returning an anonymous function that expects two arguments: the accumulator and one anonymous function. Therefore, actual stream could work like this:

def actual_stream(...) do
  fn fun, acc ->
    conn = proxy_process_checkout!(...)
    Conn.request(..., acc, fun)
  end
end

And that would allow us to reuse most of the infrastructure that is already in place. The differences are:

1. Today acc is the value while in streams it is in the format {:cont, acc}, {:halt, acc}, {:suspend, acc}. However, you can change the existing implementation to pass {:cont, acc} tuples instead and implement the other formats.

2. Today the request function does not implement an after callback. Some code to execute when the stream halts or the invoked function raises. You could add one optional argument. So overall, the stream would do:

def actual_stream(...) do
  fn fun, acc ->
    conn = proxy_process_checkout!(...)
    Conn.request(..., acc, fun, fn -> ...after... end)
  end
end

I think this direction would likely require fewer changes to the codebase and make sure that all request and stream implementations use the same code paths.

[hissssst]

All done, I've also implemented failsafe check for stream which were lost and not started

[wojtekmach]

I wonder if it is possible to expose some of these internals publicly and create a stream on top of these. Say we have a LiveView and we use Finch.async_request today. My understanding is since it’s a firehose it could overflow the LiveView process to the point it’s unresponsive to handle_event and similar. If there is something like a blocking Finch.stream_next, returning something like :more/:done/etc, a LV could send a message to itself to stream next response chunk but otherwise be responsive to any other message. Or is there a selective receive for handle event somewhere to make sure it is handled first and this is mostly moot?

[hissssst]

@wojtekmach

My understanding is since it’s a firehose it could overflow the LiveView process to the point it’s unresponsive to handle_event and similar

Yes, I've pointed this out here. We don't need stream_next. The better thing would be a behaviour like active: once, because it will copy the message into process only once.

But today we already have a solution for this problem (but it copies the response twice). You just need to spawn a process doing Finch.stream, where callback sends message to the caller and then waits in the receive for notification to send a new message.

So this PR won't solve the problem you're describing. We need to do active: once with a similar pattern. However, all of this feels like a NimblePool limitation, which has only transactional-callback interface, while it is clear now that manual checkout and checkin functions would be a more friendly interface for these kinds of problems. Maybe the NimblePool was not the best choice for the library in the first place, but I am not 100% sure about this claim

[hissssst]

@josevalim , I've implemented your suggestions, you can check the implementation. I didn't implement automatic retry though, but I think it's not a blocker. The only problem is that all failsafe timeout tests are passing until the last line and then dying with exit shutdown during on_exit or something and I have no idea why

[josevalim]

Thank you. I am swamped with ElixirConf, can you please remind me in 2 weeks if I did not comment before?

[hissssst]

@josevalim ping as requested

[josevalim]

@hissssst your ping has been on my inbox since Oct 25th. I am aware of it, I just wanted to let you know it was not ignored. Sorry.