Skip to content
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

Move error handling to C code #265

Merged
merged 10 commits into from
Mar 18, 2025
Merged

Move error handling to C code #265

merged 10 commits into from
Mar 18, 2025

Conversation

qmuntal
Copy link
Collaborator

@qmuntal qmuntal commented Mar 12, 2025

This PR moves most of the error handling to the C side so that we don't have to worry about us retrieving OpenSSL errors from the wrong OpenSSL per-thread error queue (see #231).

The main trick is to autogenerate a new set of C wrappers, for those functions that return an error, with an additional error parameter that will be filled with error context if the function fails. The Go autogenerated functions will convert that error context into a Go error and return it to the caller.

This new error handling approach should have 0 performance impact when there are no errors. In the error situation, expect less allocations thanks to some small improvements I've done to the C ->Go error mapping process:

goos: windows
goarch: amd64
pkg: github.com/golang-fips/openssl/v2
cpu: Intel(R) Core(TM) i7-10850H CPU @ 2.70GHz
         │   old.txt   │               new.txt               │
         │   sec/op    │    sec/op     vs base               │
Error-12   5.035µ ± 1%   4.685µ ± 13%  -6.96% (p=0.019 n=10)

         │   old.txt    │               new.txt                │
         │     B/op     │     B/op      vs base                │
Error-12   2.184Ki ± 0%   1.537Ki ± 0%  -29.61% (p=0.000 n=10)

         │  old.txt   │              new.txt               │
         │ allocs/op  │ allocs/op   vs base                │
Error-12   20.00 ± 0%   10.00 ± 0%  -50.00% (p=0.000 n=10)

Note that this PR almost only changes mkcgo and autogenerated code, as #262 already did the job of refactoring the codebase to construct Go errors in the autogenerated code.

Fixes #231.

@qmuntal qmuntal changed the title move error handling to C code Move error handling to C code Mar 12, 2025
@qmuntal qmuntal marked this pull request as ready for review March 12, 2025 10:19
Copy link
Collaborator

@dagood dagood left a comment

Choose a reason for hiding this comment

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

Nice! A small handful of minor suggestions.

@gdams gdams merged commit 1ce1225 into v2 Mar 18, 2025
50 checks passed
@gdams gdams deleted the gencerror branch March 18, 2025 17:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Error handling is not thead-safe
4 participants