Skip to content

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
gdams pushed a commit to gdams/openssl that referenced this pull request Jul 14, 2025
* move error handling to C code

* add error test

* fix TestErrorAllocs

* undo version loading

* fix TestErrorMultithread

* autogenerate mkcgoNoEscape

* add nosplit

* Apply suggestions from code review

Co-authored-by: Davis Goodin <dagood@users.noreply.github.com>

* decouple error implementation

* improve go_hash_sum error messages

---------

Co-authored-by: Davis Goodin <dagood@users.noreply.github.com>
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

5 participants