Fix SocketStream.is_readable()#242
Conversation
povilasb
force-pushed
the
fix/socket-readability
branch
2 times, most recently
from
a585bd0 to
5f04618
Compare
|
Rebased off latest |
florimondmanca
left a comment
florimondmanca
left a comment
There was a problem hiding this comment.
Looking good! We haven't been using mock-based unit tests very much yet. Usually we use a "fakes" approach, where we pass actual objects implementing the interfaces we're testing against. But in this case I agree monkey patching seems like the simplest approach.
tests/backend_tests/test_asyncio.py
Outdated
| @pytest.mark.asyncio | ||
| async def test_returns_true_when_socket_is_readable(self): | ||
| stream_reader = MagicMock() | ||
| stream_reader._transport.get_extra_info.return_value = None |
There was a problem hiding this comment.
Hmm. Should this be returning something truthy? Right now the two tests do the same thing, and the patched is_socket_readable helper isn't actually called.
There was a problem hiding this comment.
you're right, let me fix this :)
florimondmanca
left a comment
florimondmanca
left a comment
There was a problem hiding this comment.
Left a few nits, but I'm happy that this successfully fixes the issue, so approving in advance. Let me know if my comments make sense, and we can merge when ready!
httpcore/_backends/asyncio.py
Outdated
| sock: Optional[socket.socket] = transport.get_extra_info("socket") | ||
| # If socket was detatched from the transport, most likely connection was reset. | ||
| # Hence make it readable to notify users to poll the socket. | ||
| return not sock or is_socket_readable(sock.fileno()) |
There was a problem hiding this comment.
(Nit) Using explicit None checks:
| return not sock or is_socket_readable(sock.fileno()) | |
| return sock is None or is_socket_readable(sock.fileno()) |
transport.get_extra_info('socket') may return None when connection is dropped.
This commit makes sure we expect such case.
Also, that means socket is readable so that the stream user would be notified/awaken and act accordingly.
Had to update the way is_socket_readable is referenced, otherwise Python
wouldn't patch it.
povilasb
force-pushed
the
fix/socket-readability
branch
from
5f04618 to
11aed6c
Compare
|
Addressed your comments and amended appropriate commits ;) |
This is quite low level tests. It also required more testing dependencies. Not sure how you feel about this, so commited separately.
povilasb
force-pushed
the
fix/socket-readability
branch
from
11aed6c to
17bc818
Compare
2 participants
transport.get_extra_info('socket') may return None when connection is dropped.
This commit makes sure we expect such case.
Also, that means socket is readable so that the stream user would be notified/awaken and act accordingly.
Fixes #239