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

Handling data coming over relay #94

Closed
wants to merge 6 commits into from
Closed

Handling data coming over relay #94

wants to merge 6 commits into from

Conversation

arkgil
Copy link
Contributor

@arkgil arkgil commented Apr 20, 2017

Addresses #77

Added API for installing handlers for incoming data. Based on that I've added simple receive functionality, both blocking and asynchronous.

@jerboabot
Copy link
Collaborator

Ebert has finished reviewing this Pull Request and has found:

  • 1 fixed issue! 🎉

You can see more details about this review at https://ebertapp.io/github/esl/jerboa/pulls/94.

@arkgil arkgil requested review from rslota and erszcz April 20, 2017 08:21
@erszcz
Copy link
Member

erszcz commented Apr 20, 2017

The code looks pretty good. I have some questions regarding the design, though.

Why is a reference the preferred way to identify a handler? When the reference is lost, due to e.g. a process crash, such a handler can't be removed anymore. This might result in dangling handlers. I think the question here really is: given a handler identified by {M, F, A} (module, function, arity), do we want to be able to register it at most once (no need for a reference, {M, F, A} is enough), once per registering process (no need for a reference, registering process's pid and {M, F, A} is enough), or many times from a single process (one {M, F, A}, one pid, multiple references to discern between the process's different registered handlers)?

Moreover, the way stream_to is implemented will lead to dangling handlers in case of the stream consumer dying (and in case of a crash we can't count on terminate being called), as the server is oblivious to the body of the implicit handler defined in stream_to. In other words, the server doesn't know it's sending messages and it also doesn't know when the recipient goes away. Maybe it's a use case for Process.monitor/1 and a more direct stream_to implementation? Otherwise we avoid the need for storing monitor references and handling DOWN messages, but also avoid the chance for cleaning the handlers up.

@arkgil
Copy link
Contributor Author

arkgil commented Apr 21, 2017

Hm, these are all valid concerns. I may have gone with too simple approach, which gives a lot of freedom and flexibility but also leaves the client process in invalid state (say, when the process we stream to dies).

I think that any solution which involves passing {M, F, A} sacrifices an ease of using a client (for example when we want to try it out in the console), although it's subjective.

I like the idea about monitoring the processes which install handlers, and cleaning handlers up if/when they die. On top of that we could change an implementation of stream_to so that it always sends data to the calling process, and given that we will monitor it, there won't be a situation when we send data to a dead process. The only disadvantage in this scenario is that we lose the flexibility of current stream_to implementation, i.e. we can't stream to other processes than the caller. I think that more advanced solution to problems of stream_to would too specific to allow us to build it on top of handlers API.

I also have a question about monitoring processes which execute handlers. Right now each handler is run in parallel using Task.start which makes the client process ignore its result, whether it crashes or not. Do you think that's a valid approach? When I was implementing that I thought that prevents us from unexpected crashes because of invalid handlers, but it doesn't give any feedback to the process which installed the failing handler.

@arkgil
Copy link
Contributor Author

arkgil commented Apr 24, 2017

Closing in favour of #96

@arkgil arkgil closed this Apr 24, 2017
@erszcz
Copy link
Member

erszcz commented Apr 24, 2017

Hmmm, rather #96, isn't it?

@arkgil arkgil deleted the data-receive branch April 25, 2017 08:49
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 this pull request may close these issues.

None yet

3 participants