-
-
Notifications
You must be signed in to change notification settings - Fork 32.3k
gh-99813: Start using SSL_sendfile
when available
#99907
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
base: main
Are you sure you want to change the base?
Conversation
Start using SSL_sendfile when available
SSL_sendfile
when available
Conflicts: Modules/clinic/_ssl.c.h
@gpshead could you please review this when you have a chance? |
I've recently pushed some changes after testing the new functionality manually by sending small and large files from Linux with the |
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 won't have time to look at this in detail for a while. I'm just dropping a note about one issue seen while quickly skimming it.
@picnixz OpenSSL provides |
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 feel we need explicit functions for common error messages just to always use the same ones (but it's a separate issue)
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'd like @ZeroIntensity to have a look for threads safety and @gpshead to confirm the implementation as well. We could add a What's New entry but it's more of an implementation detail so I would prefer not to in the first place.
@@ -74,6 +74,33 @@ | |||
#endif | |||
|
|||
|
|||
#ifdef BIO_get_ktls_send | |||
# ifdef MS_WINDOWS | |||
typedef long long Py_off_t; |
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 think I've seen this construction elsewhere. This is off topic but I think we should add it to the default clinic converters or be able to share code like that across clinic generated code. I need to think about it later.
int uses = BIO_get_ktls_send(SSL_get_wbio(self->ssl)); | ||
// BIO_get_ktls_send() returns 1 if kTLS is used and 0 if not. | ||
// Also, it returns -1 for failure before OpenSSL 3.0.4. | ||
return Py_NewRef(uses == 1 ? Py_True : Py_False); |
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.
This is an immortal reference, you can omit Py_NewRef
.
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.
Ideally it's better to make it explicit. We had a similar discussion on the discord and on some issues
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 thought the consensus was to omit immortal reference counting.
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 think there was no clear consensus but I think it's better to use the macro when possible and use Py_NewRef otherwise, even if the macro does not use Py_NewRef. IIRC, the discussion involved Irit and Serhiy so if you can find it and see what we eventually decided, it would be great! (I don't have the time nor the resources for that). And if I incorrectly remembered the consensus then I apologise in advance!
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.
It started with returning Py_False
without the wrapper as it looked like the most clean option, than I was asked to use Py_NewRef
and Py_RETURN_FALSE
.
"There should be one-- and preferably only one --obvious way to do it." is not applicable to returning booleans in CPython's C code definitely 😄
Misc/NEWS.d/next/Library/2023-03-13-22-51-40.gh-issue-99813.40TV02.rst
Outdated
Show resolved
Hide resolved
int has_timeout; | ||
|
||
if (sock != NULL) { | ||
if ((PyObject *)sock == Py_None) { |
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.
Not a real issue, but thinking out loud for me and Bénédikt: does Py_Is
come with _PyObject_CAST
? If so, it seems more useful than ==
here. If not, we should add it.
Co-authored-by: Peter Bierma <zintensitydev@gmail.com>
@ZeroIntensity @picnixz thanks for the suggestion! I applied them |
I'm very sorry but I'm travelling and I'm not back before the beta release. Since features are only accepted until then, it will need to wait for 3.15 unless @gpshead reviews, accepts and merges this. |
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 forgot to approve this, LGTM as well.
@picnixz is it possible to merge it for 3.15 now? |
For me it's ok but @gpshead hasn't re-reviewed it yet so I'm waiting. Note: 3.15 has many alphas so it will anyway be merged IMO. |
SSL_sendfile()
from OpenSSL 3.0 inSSLSocket.sendfile
when available. #99813New
_ssl__SSLSocket_sendfile_impl
function is very similar to_ssl__SSLSocket_write_impl
with a few adjustments for theSSL_sendfile
syscall.I am glad there was non-SSL
socket.socket.sendfile
and underlyingsocket.socket._sendfile_use_sendfile
implemented. I reused that code for newssl.SSLSocket._sendfile_use_ssl_sendfile
by moving shared logic tosocket.socket._sendfile_zerocopy
.And
test.test_ssl.ThreadedTests.test_sendfile
needed to have a socket bound before an SSL context wrapped it because this seemed to affect kTLS availability.