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

Feature: Response callback fun #314

Open
zuiderkwast opened this issue Jul 4, 2023 · 4 comments · May be fixed by #329
Open

Feature: Response callback fun #314

zuiderkwast opened this issue Jul 4, 2023 · 4 comments · May be fixed by #329

Comments

@zuiderkwast
Copy link
Contributor

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:

  • New option #{callback_fun => CallbackFun} in ReqOpts.
  • Instead of ReplyTo ! {gun_response, ...}, the callback is used as CallbackFun(ReplyTo, {gun_response, ...})

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
Copy link
Member

essen commented Jul 4, 2023

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
Copy link
Contributor Author

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
Copy link
Member

essen commented Jul 4, 2023

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

@zuiderkwast
Copy link
Contributor Author

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.

@zuiderkwast zuiderkwast linked a pull request Jan 12, 2024 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants