RFC: Do Data operations within a locked thread#47
RFC: Do Data operations within a locked thread#47mtrmac wants to merge 1 commit intoproglottis:masterfrom
Conversation
Luap99
left a comment
There was a problem hiding this comment.
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?
|
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. |
but that’s the strongest I can find from the time this locking was introduced. |
|
Updated to call I think this is ready to go — other than needing #46 to be merged first. |
|
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. |
|
No worries, the discovered problems within |
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>
|
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. |
See #45 for more discussion.
RFC:
This bumps Go to 1.18 because I used generics. That is not really necessary.This is on top of #46 .