GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — gh-123954: proposal for improving logic for setting SSL exceptions by picnixz · Pull Request #128391 · python/cpython

Conversation

@picnixz

@picnixz picnixz commented Jan 1, 2025 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

I'd recommend also reviewing each commit separately due to the changes arising from logic extraction. I've also observed that we incorrectly use ERR_GET_REASON(p) on a custom error code. It's not really an issue because this is a no-op but I've added an assertion and removed the call for the semantics to match (we format the message using the error coming from ERR_peek_last_error() but the exception object is initialized with another error code that can also be obtained by SSL_get_error() or is a custom one).

I haven't benchmarked the branch, I just want to share a proposal. Note that refactoring the logic could help target the operations that take longer time.

cc @tarasko @kumaraditya303 @gpshead

  • Issue: Inefficient ssl.SSLWantReadError exception slows down very common use-case #123954

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
picnixz added 10 commits December 31, 2024 14:14
@picnixz
introduce semantic types for SSL packed errors
da3968c
@picnixz
correct error deduction in fill_and_set_sslerror and _setSSLError
704e6e3
@picnixz
pre-initialize default exception attributes
864339a
@picnixz
extract logic for fetching library and reason
87a501d
@picnixz
extract logic for formatting error message
7cb16c0
@picnixz
extract logic for building simple error message
4bd14a0
@picnixz
extract logic for building SSL verification exceptions
c29f0f1
@picnixz
upgrade logic for building SSL exceptions
d8de992
@picnixz
fix various issues
ba1bf20
@picnixz
use PyOS_snprintf instead of PyUnicode_FromFormat
11ee17c
@bedevere-app bedevere-app Bot mentioned this pull request Jan 1, 2025
Inefficient ssl.SSLWantReadError exception slows down very common use-case #123954
Open
@picnixz picnixz added the skip news label Jan 1, 2025
@picnixz
fix Windows compilation
45c7a1e
ZeroIntensity
ZeroIntensity reviewed Jan 1, 2025
View reviewed changes
Comment thread Modules/_ssl.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

picnixz
picnixz commented Jan 1, 2025
View reviewed changes
Comment thread Modules/_ssl.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Modules/_ssl.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Modules/_ssl.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Modules/_ssl.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Modules/_ssl.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Modules/_ssl.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

@picnixz
Apply suggestions from code review
4362dae
picnixz
picnixz commented Jan 1, 2025
View reviewed changes
Comment thread Modules/_ssl.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

@picnixz
Update Modules/_ssl.c
31ce5f0
@picnixz

picnixz commented Jan 1, 2025

Copy link
Copy Markdown
Member Author

Since all strings that will be rendered in the exception are actually ASCII messages (no localization), I'm considering switching from a PyDict storage to a _Py_hashtable_t structure but I don't know which one is the most efficient. With a _Py_hashtable_t structure, I can simply forget about Unicode PyObject * objects and deal with const char * objects only.

I haven't checked more in details though since I already want to know whether the proposal performs well.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@ZeroIntensity

ZeroIntensity commented Jan 1, 2025

Copy link
Copy Markdown
Member

_Py_hashtable_t will probably perform better, but it's not thread safe: is it possible for concurrent writes to be an issue here?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@picnixz

picnixz commented Jan 1, 2025 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member Author

No, we're only reading from the hash table. It is cached in the module's state and we only lookup strings by integer keys or pairs of integers.

ZeroIntensity reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

picnixz added 9 commits January 2, 2025 10:26
@picnixz
post-merge
2fbcae2
@picnixz
use _Py_hashtable_t for err_codes_to_names
fd7d3ec
@picnixz
use _Py_hashtable_t for lib_codes_to_names
7772113
@picnixz
post-merge
4a2ed1d
@picnixz
fix compilation?
630c2b5
@picnixz
attempt various things
98c8b99
@picnixz
try even more things!
133ec9f
@picnixz
:@
fb5d800
@picnixz
:@@@@@@@@@@@@@@
d6a0529
picnixz added 4 commits January 5, 2025 15:29
@picnixz
select when to use verbose SSL errors
b8f0cce
@picnixz
reduce the number of format units
689568f
@picnixz
optimize format_ssl_error_message
5583aa5
@picnixz
temporarily patch CI
a0ab1c2
@picnixz
picnixz force-pushed the perf/ssl/error-123954 branch from e9d3230 to 522da2b Compare January 5, 2025 14:41
@picnixz
fix assertions
6b38769
@picnixz
picnixz force-pushed the perf/ssl/error-123954 branch from 522da2b to 6b38769 Compare January 5, 2025 14:42
picnixz added 4 commits January 5, 2025 15:59
@picnixz
now fix assertions correctly
3ecba48
@picnixz
use ImportWarning insteaad of RuntimeWarning
d7f1ab1
@picnixz
clear exception after emitting the warning
32f7cba
@picnixz
fix error path when formatting message
a88ec91
@ZeroIntensity

ZeroIntensity commented Jan 9, 2025

Copy link
Copy Markdown
Member

OpenSSL has fixed the issue upstream, but it looks like the release isn't due until April.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@picnixz
picnixz marked this pull request as ready for review January 10, 2025 14:04
@bedevere-app bedevere-app Bot added the awaiting review label Jan 10, 2025
@picnixz

picnixz commented Jan 10, 2025

Copy link
Copy Markdown
Member Author

Oups I misclicked on my mobile app. It's still a draft.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@ZeroIntensity
ZeroIntensity marked this pull request as draft January 10, 2025 14:11
@bedevere-app bedevere-app Bot removed the awaiting review label Jan 10, 2025
@ZeroIntensity

ZeroIntensity commented Jan 10, 2025

Copy link
Copy Markdown
Member

Converted it back for you. IIRC the GitHub app doesn't let you mark something as a draft.

picnixz reacted with heart emoji
All reactions
  • ❤️ 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

picnixz added 2 commits January 11, 2025 11:38
@picnixz
Merge remote-tracking branch 'upstream/main' into perf/ssl/error-123954
a9a6a50
@picnixz
include hexadecimal error code value when reporting unknown error
68f8bc7
@picnixz

picnixz commented Jan 11, 2025

Copy link
Copy Markdown
Member Author

Btw, I think we also have another issue with another code:

:1058: ImportWarning: SSL data contains incompatible entries for (41, 104; 0x14800068, 343933032). Old mnemonic is BROTLI_INFLATE_ERROR but new mnemonic is BROTLI_NOT_SUPPORTED.

I didn't check whether this is still the case, but it's probably worth to check as well.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@ZeroIntensity

ZeroIntensity commented Jan 11, 2025

Copy link
Copy Markdown
Member

Frustratingly, looks like OpenSSL again: https://github.com/openssl/openssl/blob/b049ce0e354011be075e620b9ba7cf4d7c8f9577/crypto/err/openssl.txt#L414 and https://github.com/openssl/openssl/blob/b049ce0e354011be075e620b9ba7cf4d7c8f9577/include/openssl/comperr.h#L28

I guess I'll go pester them again. Are there any other cases like this, or is this the last one?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@picnixz

picnixz commented Jan 11, 2025

Copy link
Copy Markdown
Member Author

I guess I'll go pester them again. Are there any other cases like this, or is this the last one?

I think it's the last one. At least, I don't see more warnings in the CI.

ZeroIntensity reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@ZeroIntensity ZeroIntensity mentioned this pull request Jan 11, 2025
Mismatch between several other macros in openssl.txt and comperr.h openssl/openssl#26388
Closed
@ZeroIntensity

ZeroIntensity commented Jan 11, 2025

Copy link
Copy Markdown
Member

I found a few others while reporting the issue, FWIW.

  • Mismatch between several other macros in openssl.txt and comperr.h openssl/openssl#26388
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@picnixz

picnixz commented Jan 11, 2025

Copy link
Copy Markdown
Member Author

I found a few others while reporting the issue,

Thanks! I think we don't use those, hence I didn't have the warning (or maybe we're using it and something else is wrong in my warnings being emitted?)

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@picnixz picnixz mentioned this pull request Mar 18, 2025
Update OpenSSL versions for CI and Windows #131423
Closed
4 tasks done
@picnixz

picnixz commented Feb 7, 2026

Copy link
Copy Markdown
Member Author

I'm going to abandon that specific PR. I may at some point refactor SSL but it's more pressing to fix other issues than this specific one (it didn't give much improvement and didn't really fix the issue users have).

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@picnixz picnixz closed this Feb 7, 2026
@picnixz
picnixz deleted the perf/ssl/error-123954 branch February 7, 2026 11:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ZeroIntensity ZeroIntensity ZeroIntensity left review comments
@gpshead gpshead Awaiting requested review from gpshead gpshead will be requested when the pull request is marked ready for review gpshead is a code owner

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

skip news

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

2 participants

@picnixz @ZeroIntensity
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.