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

Example client exits on ConnectionClosed (#22) #55

Merged
merged 1 commit into from
Oct 23, 2018

Conversation

mehaase
Copy link
Contributor

@mehaase mehaase commented Oct 16, 2018

This is the second attempt at this. Instead of modifying
trio-websocket to support cancelling a handler, this commit
modifies only the example client itself: it now has one task
that gets messages and prints them, and a second task that
gets user input and sends messages/commands/etc.

This supersedes #48.

This is the second attempt at this. Instead of modifying
trio-websocket to support cancelling a handler, this commit
modifies only the example client itself: it now has one task
that gets messages and prints them, and a second task that
gets user input and sends messages/commands/etc.
@belm0
Copy link
Member

belm0 commented Oct 17, 2018

I'm not a fan of splitting send and receive to resolve this issue, as it's creating another state machine. We should use concurrency to avoid coding explicit state machines (http://250bpm.com/blog:69). In a real program doing query-with-reply over websocket, it's much simpler to keep the send and receive within a single imperative control flow.

If the heartbeat pattern from the other CL works I'd rather this example use it, and keep the send / receive together.

@mehaase mehaase mentioned this pull request Oct 17, 2018
@mehaase
Copy link
Contributor Author

mehaase commented Oct 17, 2018

Copying @belm0 from the "ping v2" thread:

What I'd like to see:

  • Call receive directly after send rather than having reader task
  • Change heartbeat to 1s interval and remove the fail_after. (I.e for quickly detecting closed connection only.)

Removing the reader task can't be done without changing the library's implementation, though, i.e. adding one of the "connection is closed" signals we talked about. Or are you suggesting the heartbeat is used for detecting a closed connection? If so, that amounts to polling the connection state (and it generates unnecessary packets if all we care about is detecting that the local socket closed), which is a pattern I don't want to encourage.

I did read the blog post you sent about state machines, but I must confess that I can't grasp its implications for this PR. The example client feels pretty simple to me. I don't see any drawback to having a separate reader task. Indeed, I think many applications that multiplex requests and responses on a single WebSocket will need to have a separate reader task, so it's not a pattern we want to avoid.

@mehaase
Copy link
Contributor Author

mehaase commented Oct 22, 2018

@belm0 Unless you strongly object, I am going to merge this as-is. I don't see where the added state machine is, and I think that separate readers and writers will be a common set up for users.

@belm0
Copy link
Member

belm0 commented Oct 22, 2018

Adding state would likely be the next logical step: the reader task would need to know what to expect on the line, e.g. if it becomes responsible for dealing with the application-level protocol.

Being an example, I think it's important to minimize the number of tasks and keep things as local as possible, and hence easy to follow. There is a class of websocket use where one side drives the messaging, and this message echo toy falls under that. So it would be great to keep the send and receive together.

Keeping that simplicity seems more important than the ideal of having the client automatically shut down on a closed connection, so I'd opt to leave things as is given a choice between the two.

I'll defer to you-- thanks for considering.

@njsmith
Copy link
Member

njsmith commented Oct 23, 2018

Of course it's just an example, so it doesn't really have to meet any particular spec. But I was imagining it as a kind of websocket test client: if it lets you connect, send arbitrary messages, and has a background loop running that prints any messages it receives, then that's sufficient to manually implement any websocket protocol, including ones that don't follow a strict request/response pattern. (And presumably most websocket protocols don't follow strict request/response patterns, since that's what HTTP does!)

@mehaase mehaase merged commit 51ad398 into master Oct 23, 2018
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.

3 participants