ninenines / gun

HTTP/1.1, HTTP/2, Websocket client (and more) for Erlang/OTP.
ISC License
891 stars 232 forks source link

Feature: Response callback fun #314

Open zuiderkwast opened 12 months ago

zuiderkwast commented 12 months ago

We would like to be able to provide a callback that handles the response, rather than only a reply_to pid, in ReqOpts in gun:request/6.

Suggested interface:

Alternatively, allow ReplyTo to be a fun instead of a pid.

Why not just use reply_to pid?

Our worker process handles messages from many different sources. It is not supposed to pattern match on gun-specific messages like {gun_response, ...} and {gun_error, ...} because it breaks the abstractions.

The callback fun would in this case just translate the message to a different form before sending it to the worker process. Today we have a temporary proxy process for this, which we want to get rid of.

Another benefit of a callback fun is that it can filter messages (ignore some responses or errors) and do simple things like logging directly from the gun worker process, to avoid one hop of message passing.

What do you think? If it is acceptable, I will submit a PR.

essen commented 12 months ago

I would like to see a better fledged interface proposal. Know which messages it is expected to replace, how it deals with data or trailers and so on.

I would favor changing reply_to to accept a {fun ..., St}. I don't know if that St should be allowed to change or not. Let's see if there's any benefits one way or the other. The St would be a way to give the Pid if you use a fun F/A form.

zuiderkwast commented 12 months ago

If reply_to can be just a fun, then the fun can encapsulate a custom pid, like

reply_to => fun(Response) -> my_response_handler(MyPid, Response) end

I think it is almost equivalent to

reply_to => {fun my_response_handler/2, MyPid}

I don't mind the latter, but I don't think it is necessary. If St is allowed to change, then the fun use it to accumulate the response until IsFin is true, but I think it might be too complex. The user can do that in their own process instead, so we don't complicate the interface too much.

essen commented 12 months ago

Those funs don't survive module reloads. So we need to have a tuple at least as an option.

zuiderkwast commented 12 months ago

Interesting. I didn't think about funs that become invalid when their module's old code is purged. Is it only a fun with explicit module like fun M:F/A that can survive a purge, i.e. not even fun F/A can survive?

Regarding the possibility to update St, we don't need it now but I think we can't add it later if we don't add in from the start.

So how about this proposal: reply_to => {Fun, St} where Fun is called as Fun(Response, St). If it returns {ok, NewSt} we keep NewSt, but otherwise (any other return value) the old St is kept.

To keep the inteface simple, it would receive exactly the same messages as the existing ReplyTo pid does.