Skip to content

bpo-35589: Prevent buffer copy in sock_sendall()#11418

Merged
miss-islington merged 3 commits into
python:masterfrom
asvetlov:fast-sendall
May 16, 2019
Merged

bpo-35589: Prevent buffer copy in sock_sendall()#11418
miss-islington merged 3 commits into
python:masterfrom
asvetlov:fast-sendall

Conversation

@asvetlov

@asvetlov asvetlov commented Jan 2, 2019

Copy link
Copy Markdown
Contributor

No NEWs is needed since the problem was introduced on master only and never released.

https://bugs.python.org/issue35589

@eamanu eamanu left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@eamanu

eamanu commented Jan 3, 2019

Copy link
Copy Markdown
Contributor

Some tests are not required?

@asvetlov

asvetlov commented Jan 3, 2019

Copy link
Copy Markdown
Contributor Author

There are tests for sending large data already.
An explicit test for memory consumption is not robust.
We can add a mocked test but the test is useless almost: it checks mocks correctness only :)

Comment thread Lib/asyncio/selector_events.py Outdated
Comment thread Lib/asyncio/selector_events.py Outdated
functools.partial(self._sock_write_done, fd))
self.add_writer(fd, self._sock_sendall, fut, sock, data)
# use a trick with a list in closure to store a mutable state
self.add_writer(fd, self._sock_sendall, fut, sock, data, [n])

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But why do we want to have a mutable state here?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I cannot pass just n -- need either a mutable object or re-add a writer after every sock.send() call until all data is sent.

@methane methane left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Theorically speaking, memoryview should be closed after using.
But it is ignored often. Unlike file.close(), not closing memoryview is not a real problem on PyPy.

I want portable version of os.pwrite(fd, buf, start), instead of abusing memoryview though...

@miss-islington
miss-islington merged commit 6e78900 into python:master May 16, 2019
@asvetlov

Copy link
Copy Markdown
Contributor Author

Sorry, forgot to merge it 3 months ago

@asvetlov
asvetlov deleted the fast-sendall branch May 16, 2019 13:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants