Skip to content

Conversation

@mtrmac
Copy link
Contributor

@mtrmac mtrmac commented Nov 5, 2025

See #45 for more discussion.

RFC:

  • I have not really exercised this code other than existing tests
  • I’d like to have more reviewers (@Luap99 PTAL)
  • This bumps Go to 1.18 because I used generics. That is not really necessary.

This is on top of #46 .

Copy link

@Luap99 Luap99 left a comment

Choose a reason for hiding this comment

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

style do we gain anything by providing these callbacks?
It seems to me the normal

runtime.LockOSThread()
defer runtime.UnlockOSThread()

at the top of each function would be preferable from a readability standpoint. It does mean the thread is locked for longer but I doubt that would actually have any noticeable impact. Also in case of Write() where it is called in a loop it means we lock/unlock over and over again which may result in more overhead as it needs to use atomics to synchronize?

@mtrmac
Copy link
Contributor Author

mtrmac commented Nov 5, 2025

It turns out that CGo is, internally, already locking Go callbacks to the thread that invoked them: https://github.com/golang/go/blob/8cf7a0b4c956aad5a8b98efde8ea6b7cde173902/src/runtime/cgocall.go#L318 .

So, this patch should not make a difference in practice — although, in principle, I don’t see that that this is externally promised, so locking ourselves still seems prudent.

@mtrmac
Copy link
Contributor Author

mtrmac commented Nov 5, 2025

I don’t see that that this is externally promised

golang/go#21827 (comment) :

seems like a good idea to preserve any C __thread variables when calling Go -> C -> Go -> C.

but that’s the strongest I can find from the time this locking was introduced.

@mtrmac
Copy link
Contributor Author

mtrmac commented Nov 12, 2025

Updated to call LockOSThread manually, without the update to Go 1.18, and to update documentation to with the more recent research.

I think this is ready to go — other than needing #46 to be merged first.

@mtrmac mtrmac marked this pull request as ready for review November 12, 2025 18:55
@proglottis
Copy link
Owner

Thanks so much for working on this @mtrmac - You'll have to forgive the delay here, I'm away, but I'll look into it as soon as I'm back.

@mtrmac
Copy link
Contributor Author

mtrmac commented Nov 13, 2025

No worries, the discovered problems within gpgme itself will take a fair amount of time to fix.

gpgme_err_set_errno means the error state is returned
in thread-local storage, so we must ensure the goroutine
does not migrate while a gpgme_data_t is used for I/O.

This should not make a difference in practice, Go
is already locking the thread when invoking callback from CGo:
https://github.com/golang/go/blob/8cf7a0b4c956aad5a8b98efde8ea6b7cde173902/src/runtime/cgocall.go#L318
but that behavior seems not to be promised anywhere.

Signed-off-by: Miloslav Trmač <mitr@redhat.com>
@mtrmac
Copy link
Contributor Author

mtrmac commented Nov 25, 2025

Rebased to be a stand-alone PR.

I think this does not ultimately change behavior or close any race, given the current Go implementation — but Go does not promise to lock the thread for us, so it would be safer to do that explicitly.

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.

3 participants