Skip to content

Conversation

@mattyoungberg
Copy link
Contributor

Fix #1552

NOTE: I added a couple of tests, but one of the two is still not passing. My intention is to fix and then squash the fix into this commit. At which point, this note will be removed.

The docstring for Connection.recv suggests that if you provide the argument timeout=0, you can retrieve any message in waiting:

"Set timeout to 0 to check if a message was already received."

Current behavior is that this does nothing and instead makes a TimeoutError immediately throw on the first use of a derived Deadline.

However, being able to check for a full message without blocking unneccesarily (aka, waiting for the server to send a message) would be useful, especially if your websocket client needs to be able to continue to do work in another thread and so periodically ceding control is important.

Most of the problem comes from the fact that the code doesn't treat a timeout value of 0 as special, and the code path will create a Deadline with it, and then rely on the side effects of blocking network I/O and exceptions in order to guarantee the contract of Connection.recv() which is to ultimately deliver a full message.

The error has been addressed by allowing the API of Assembler.get() to potentially return None when a new param, peek, is set to True (the default is False and maintains previous behavior). The intention of this param is to allow the method to acquire the class's mutex, check if there are any frames in the Assembler instance, and if not, return None. The only place where this param is used is within Connection.recv(), which is now able to treat the exceptional case of timeout=0 by setting up it's arguments to Assembler.get() without otherwise changing its code path.

Tests:

  • Two tests were added: one to make sure that a client gets back None when timeout=0 and the underlying frames queue is empty, and another that returns the correct message when the underlying frames queue has a frame.

Fix python-websockets#1552

NOTE: I added a couple of tests, but one of the two is still not
passing. My intention is to fix and then squash the fix into this
commit. At which point, this note will be removed.

The docstring for `Connection.recv` suggests that if you provide the
argument `timeout=0`, you can retrieve any message in waiting:

"Set `timeout` to `0` to check if a message was already received."

Current behavior is that this does nothing and instead makes a
`TimeoutError` immediately throw on the first use of a derived
`Deadline`.

However, being able to check for a full message without blocking
unneccesarily (aka, waiting for the server to send a message) would be
useful, especially if your websocket client needs to be able to continue
to do work in another thread and so periodically ceding control is
important.

Most of the problem comes from the fact that the code doesn't treat a
timeout value of 0 as special, and the code path will create a
`Deadline` with it, and then rely on the side effects of blocking
network I/O and exceptions in order to guarantee the contract of
`Connection.recv()` which is to ultimately deliver a full message.

The error has been addressed by allowing the API of `Assembler.get()` to
potentially return `None` when a new param, `peek`, is set to True (the
default is False and maintains previous behavior). The intention of this
param is to allow the method to acquire the class's mutex, check if
there are any frames in the `Assembler` instance, and if not, return
`None`. The only place where this param is used is within
`Connection.recv()`, which is now able to treat the exceptional case of
`timeout=0` by setting up it's arguments to `Assembler.get()` without
otherwise changing its code path.

Tests:

- Two tests were added: one to make sure that a client gets back `None`
  when `timeout=0` and the underlying frames queue is empty, and another
that returns the correct message when the underlying frames queue has a
frame.
@mattyoungberg
Copy link
Contributor Author

Closing in favor of #1573

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.

Unclear behaviour of recv(timeout=0) for Server (threading)

1 participant