gh-99813: Start using SSL_sendfile when available
#99907
Conversation
Start using SSL_sendfile when availableSSL_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 |
|
@picnixz OpenSSL provides |
There was a problem hiding this comment.
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.
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.
This is an immortal reference, you can omit Py_NewRef.
There was a problem hiding this comment.
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.
Oh? I thought the consensus was to omit immortal reference counting.
There was a problem hiding this comment.
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.
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.
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. |
|
@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.sendfilewhen available. #99813New
_ssl__SSLSocket_sendfile_implfunction is very similar to_ssl__SSLSocket_write_implwith a few adjustments for theSSL_sendfilesyscall.I am glad there was non-SSL
socket.socket.sendfileand underlyingsocket.socket._sendfile_use_sendfileimplemented. I reused that code for newssl.SSLSocket._sendfile_use_ssl_sendfileby moving shared logic tosocket.socket._sendfile_zerocopy.And
test.test_ssl.ThreadedTests.test_sendfileneeded to have a socket bound before an SSL context wrapped it because this seemed to affect kTLS availability.