-
-
Notifications
You must be signed in to change notification settings - Fork 30.3k
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
bpo-41279: Add StreamReaderBufferedProtocol #21446
base: main
Are you sure you want to change the base?
Conversation
d594f7a
to
af23da7
Compare
af23da7
to
c533f08
Compare
c533f08
to
806b335
Compare
806b335
to
62da6f0
Compare
62da6f0
to
ac1418d
Compare
ac1418d
to
71ea0a6
Compare
Lib/asyncio/proactor_events.py
Outdated
|
||
data = self._data[:length] | ||
if length > -1: | ||
if not isinstance(self._protocol, protocols.BufferedProtocol): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we have proper functional tests that hit this branch? If not we need to add them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can probably copy some tests from the uvloop project (as I own the project there will be no licensing issues even if it's a literal copy)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am thinking of doing parameterize on all the appropriate test in test_proactor_events.py.
That way we can use the already existing tests on both Protocol and BufferedProtocol
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok I did this using the parameterize function, it did made me write a bit of ugly code I want you to look at.
Check out what I did in the test adding commit.
By the way shouldn't I add the same tests for UnixReadPipeTransportTests
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok I added the tests to UnixReadPipeTransportTests
as well using the same patches
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok I did this using the parameterize function, it did made me write a bit of ugly code I want you to look at.
Yeah, let's try to simplify it. See my other comment.
By the way shouldn't I add the same tests for UnixReadPipeTransportTests?
I guess since we're updating _UnixReadPipeTransport
we should.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I simplified it a lot
Lib/asyncio/proactor_events.py
Outdated
except (SystemExit, KeyboardInterrupt): | ||
raise |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
except (SystemExit, KeyboardInterrupt): | |
raise |
the two llines can be removed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
71ea0a6
to
2a4a9eb
Compare
Lib/asyncio/proactor_events.py
Outdated
self._fatal_error(exc, | ||
'Fatal error: protocol.buffer_updated() ' | ||
'call failed.') | ||
return |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return |
just a small change ;)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
2a4a9eb
to
38704db
Compare
Hey, just remembered that I did this, bumping! :) |
self.protocol.data_received.assert_called_with(msgs[1]) | ||
|
||
tr.resume_reading() | ||
tr.resume_reading() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why this double pause_reading()
above and then resume_reading()
here ?
and same question for below in test_pause_resume_reading
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
|
||
for msg in msgs[:2]: | ||
self.loop._run_once() | ||
self.protocol.data_received.assert_called_with(msg) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
before the _run_once()
would not it be better to assert that the msg
is not in the _mock_call_args_list
of the mock object ?
and same below for the others data_received.assert_called_with
following a _run_once()
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand which mock object you are talking about, and I don't see any other test reading _mock_call_args_list
.
Can you elaborate?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
something like:
for msg in msgs[:2]:
self.assertFalse(unittest.mock.call(msg) in self.protocol.data_received.mock_calls)
self.loop._run_once()
self.protocol.data_received.assert_called_with(msg)
to ensure that each msg
is effectively processed during the _run_once() call.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh I see, but protocol's buffer_updated
(data_received is only in a regular stream object, not buffered) is not a mock, it's an actual function, so no mock_calls
field.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah ok :/
1de298c
to
ea382bd
Compare
This class gets better performance as BufferedProtocol uses read_into into a buffer allocated before, instead of allocating a new buffer each time read is called.
…pythonGH-21446) The transport did not know how to use the proper api exported by BufferedProtocol. Added a new callback function that calls getbuffer() and buffer_updated() instead of data_received() when the protocol given to it is of type BufferedProtocol. This is exactly the same way _SelectorSocketTransport handles a BufferedProtocol.
…port (pythonGH-21446) In the __init__ function if the protocol is of instance BufferedProtocol instead of creating a buffer object, we call get_buffer on the protocol to get its buffer. In addition _loop_reading now calls _data_received as soon as there is actual data instead of calling only after adding a recv_into event. The reason for this change is because read_into could call it's callback immediatly meaning overriding the data on the buffer before we actually call _data_received on it, which fixes the potential issue of missed data.
ea382bd
to
d0dcc84
Compare
I'll fix the ssl tests sometime this weekend |
@tontinton You can turn your PR into a draft and not worry about timeframes. |
d0dcc84
to
35771ff
Compare
…pythonGH-21446) When calling set_protocol to change the protocol you can now change the type of the protocol from BufferedProtocol to Protocol or vice versa. start_tls needed this feature as it could read into a buffered protocol at first and then change the protocol to SSLProto which is a regular protocol.
35771ff
to
3dc4d18
Compare
https://bugs.python.org/issue41279
I got way better performance on
await reader.read()
using this branch on linux (check out the chart on theserver.py
script's comments).The way I tested was writing a server / client:
server.py
:client.py
: