-
Notifications
You must be signed in to change notification settings - Fork 601
Improve SSL performance by avoiding SSLWantReadError exception and using much faster checks whenever possible #629
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
Changes from 10 commits
f18d1a3
951e8df
26ede3c
eb7ce66
473214b
62272e1
7b1f810
a73d44e
1078fc2
5971ab7
4434c63
27d0693
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -480,6 +480,7 @@ cdef class SSLProtocol: | |||||
| server_hostname=self._server_hostname) | ||||||
| self._sslobj_read = self._sslobj.read | ||||||
| self._sslobj_write = self._sslobj.write | ||||||
| self._sslobj_pending = self._sslobj.pending | ||||||
| except Exception as ex: | ||||||
| self._on_handshake_complete(ex) | ||||||
| else: | ||||||
|
|
@@ -719,48 +720,91 @@ cdef class SSLProtocol: | |||||
|
|
||||||
| cdef _do_read__buffered(self): | ||||||
| cdef: | ||||||
| Py_buffer pybuf | ||||||
| bint pybuf_inited = False | ||||||
| size_t wants, offset = 0 | ||||||
| int count = 1 | ||||||
| object buf | ||||||
| Py_ssize_t total_pending = (<Py_ssize_t>self._incoming.pending | ||||||
| + <Py_ssize_t>self._sslobj_pending()) | ||||||
| # Ask for a little extra in case when decrypted data is bigger than | ||||||
| # original | ||||||
| object app_buffer = self._app_protocol_get_buffer( | ||||||
| total_pending + 256) | ||||||
| Py_ssize_t app_buffer_size = len(app_buffer) | ||||||
|
|
||||||
| if app_buffer_size == 0: | ||||||
| return | ||||||
|
|
||||||
| buf = self._app_protocol_get_buffer(self._get_read_buffer_size()) | ||||||
| wants = len(buf) | ||||||
| cdef: | ||||||
| Py_ssize_t last_bytes_read = -1 | ||||||
| Py_ssize_t total_bytes_read = 0 | ||||||
| Py_buffer pybuf | ||||||
| bint pybuf_initialized = False | ||||||
|
|
||||||
| try: | ||||||
| count = self._sslobj_read(wants, buf) | ||||||
|
|
||||||
| if count > 0: | ||||||
| offset = count | ||||||
| if offset < wants: | ||||||
| PyObject_GetBuffer(buf, &pybuf, PyBUF_WRITABLE) | ||||||
| pybuf_inited = True | ||||||
| while offset < wants: | ||||||
| buf = PyMemoryView_FromMemory( | ||||||
| (<char*>pybuf.buf) + offset, | ||||||
| wants - offset, | ||||||
| # SSLObject.read may not return all available data in one go. | ||||||
| # We have to keep calling read until it throw SSLWantReadError. | ||||||
| # However, throwing SSLWantReadError is very expensive even in | ||||||
| # the master trunk of cpython. | ||||||
| # See https://github.com/python/cpython/issues/123954 | ||||||
|
|
||||||
| # One way to reduce reliance on SSLWantReadError is to check | ||||||
| # self._incoming.pending > 0 and SSLObject.pending() > 0. | ||||||
| # SSLObject.read may still throw SSLWantReadError even when | ||||||
| # self._incoming.pending > 0 SSLObject.pending() == 0, | ||||||
tarasko marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||||||
| # but this should happen relatively rarely, only when ssl frame | ||||||
| # is partially received. | ||||||
|
|
||||||
| # This optimization works really well especially for peers | ||||||
| # exchanging small messages and wanting to have minimal latency. | ||||||
|
|
||||||
| # self._incoming.pending means how much data hasn't | ||||||
| # been processed by ssl yet (read: "still encrypted"). The final | ||||||
| # unencrypted data size maybe different. | ||||||
|
|
||||||
| # self._sslobj.pending() means how much data has been already | ||||||
| # decrypted and can be directly read with SSLObject.read. | ||||||
|
|
||||||
| # Run test_create_server_ssl_over_ssl to reproduce different cases | ||||||
| # for this method. | ||||||
| while total_pending > 0: | ||||||
| if total_bytes_read > 0: | ||||||
| if not pybuf_initialized: | ||||||
| PyObject_GetBuffer(app_buffer, &pybuf, PyBUF_WRITABLE) | ||||||
| pybuf_initialized = True | ||||||
|
|
||||||
| app_buffer = PyMemoryView_FromMemory( | ||||||
| (<char*>pybuf.buf) + total_bytes_read, | ||||||
| app_buffer_size - total_bytes_read, | ||||||
| PyBUF_WRITE) | ||||||
| count = self._sslobj_read(wants - offset, buf) | ||||||
| if count > 0: | ||||||
| offset += count | ||||||
| else: | ||||||
| break | ||||||
| else: | ||||||
|
|
||||||
| last_bytes_read = <Py_ssize_t>self._sslobj_read( | ||||||
| app_buffer_size, app_buffer) | ||||||
|
||||||
| app_buffer_size, app_buffer) | |
| app_buffer_size - total_bytes_read, app_buffer) |
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.
That's a good one. Fixed!
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.
Actually, probably doesn't make a difference because SSLObject.read won't read more than the length of app_buffer and it is already app_buffer_size - total_bytes_read. I fixed it anyway for consistency
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.
Yes, this whole SSLWantWriteError from SSLObject.read is tricky.
I'm not 100% sure but I have the following assumption:
-
Python ssl.MemoryBIO grows dynamically without fixed capacity, has no limit, write can only fail in case of system out-of-memory
-
SSL_read_ex returns SSL_WANT_WRITE_ERROR when memory BIO runs out of memory, not when we just wrote something. So given (1) SSL_WANT_WRITE_ERROR may only happen when the system runs out of memory.
-
SSL_read_ex does use its chance to write control stuff right after successful read before returning. link
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.
Ahh, good catch! This fixes a false EOF bug in previous code I think. Would be nice to have a test!