Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

gh-99813: Start usingSSL_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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Open
illia-v wants to merge54 commits intopython:main
base:main
Choose a base branch
Loading
fromillia-v:SSL_sendfile

Conversation

illia-v
Copy link
Contributor

@illia-villia-v commentedNov 30, 2022•
edited
Loading

New_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-SSLsocket.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.

Andtest.test_ssl.ThreadedTests.test_sendfile needed to have a socket bound before an SSL context wrapped it because this seemed to affect kTLS availability.

@illia-villia-v changed the titlegh-99813:Start using SSL_sendfile when availablegh-99813: Start usingSSL_sendfile when availableNov 30, 2022
@illia-villia-v marked this pull request as ready for reviewMarch 13, 2023 21:29
@illia-v
Copy link
ContributorAuthor

@gpshead could you please review this when you have a chance?

@illia-v
Copy link
ContributorAuthor

I've recently pushed some changes after testing the new functionality manually by sending small and large files from Linux with thetls module loaded, it worked well

Copy link
Member

@gpsheadgpshead left a 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.

@illia-villia-v requested a review fromgpsheadAugust 2, 2023 19:54
@illia-v
Copy link
ContributorAuthor

@picnixz OpenSSL providesSSL_OP_ENABLE_KTLS_TX_ZEROCOPY_SENDFILE which will be good to have in Python together with the change. Should I add it in this PR or separately after this one is merged?

@illia-villia-v requested a review frompicnixzApril 19, 2025 15:33
Copy link
Member

@picnixzpicnixz left a comment•
edited
Loading

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)

illia-v reacted with thumbs up emoji
@illia-villia-v requested a review frompicnixzApril 28, 2025 21:16
Copy link
Member

@picnixzpicnixz left a 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;
Copy link
Member

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);
Copy link
Member

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 omitPy_NewRef.

Copy link
Member

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

Copy link
Member

@ZeroIntensityZeroIntensityMay 1, 2025•
edited
Loading

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.

Copy link
Member

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!

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

It started with returningPy_False without the wrapper as it looked like the most clean option, than I was asked to usePy_NewRef andPy_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 😄

int has_timeout;

if (sock != NULL) {
if ((PyObject *)sock == Py_None) {
Copy link
Member

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: doesPy_Is come with_PyObject_CAST? If so, it seemsmore useful than== here. If not, we should add it.

@illia-v
Copy link
ContributorAuthor

illia-v commentedMay 2, 2025•
edited
Loading

@ZeroIntensity@picnixz thanks for the suggestion! I applied them

@picnixz
Copy link
Member

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.

Copy link
Member

@ZeroIntensityZeroIntensity left a 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.

illia-v reacted with heart emoji
@illia-v
Copy link
ContributorAuthor

@picnixz is it possible to merge it for 3.15 now?

@picnixz
Copy link
Member

picnixz commentedMay 19, 2025•
edited
Loading

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.

Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@picnixzpicnixzpicnixz approved these changes

@ZeroIntensityZeroIntensityZeroIntensity approved these changes

@gpsheadgpsheadAwaiting requested review from gpsheadgpshead is a code owner

Assignees
No one assigned
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

5 participants
@illia-v@picnixz@gpshead@ZeroIntensity@bedevere-bot
[8]ページ先頭
©2009-2025 Movatter.jp