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.

Sign up for GitHub

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

Jump to bottom

Go: Fix channel passing from Go to Rust by using runtime.Pinner or cgo.Handle #3208

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Go: Fix channel passing from Go to Rust by using runtime.Pinner or cgo.Handle #3208

wants to merge 3 commits into from
+155 −6

Conversation

rueian
Copy link

@rueian rueian commented Feb 19, 2025 •
edited
Loading

This fixes #3207 and makes the Go client deliver responses to the correct channels.

The current way of passing uintptr(unsafe.Pointer) to rust and back to Go is not safe for dereferencing. Go can still move the address.

Use runtime.Pinner or cgo.Handle to pin the channel address before passing it to the rust.

Issue link

This Pull Request is linked to issue (URL): #3207

Checklist

Before submitting the PR make sure the following are checked:

  • This Pull Request is related to one issue.
  • Commit message has a detailed description of what changed and why.
  • Tests are added or updated.
  • CHANGELOG.md and documentation files are updated.
  • Destination branch is correct - main or release
  • Create merge commit if merging release branch into main, squash otherwise.

@rueian rueian force-pushed the fix-channel-passing branch 3 times, most recently from 231989c to 00934da Compare February 19, 2025 09:29
@rueian rueian marked this pull request as ready for review February 19, 2025 09:31
@rueian rueian requested a review from a team as a code owner February 19, 2025 09:31
@rueian rueian force-pushed the fix-channel-passing branch from 00934da to 111af00 Compare February 19, 2025 09:39
@Yury-Fridlyand Yury-Fridlyand added the go golang wrapper label Feb 19, 2025
@yipin-chen yipin-chen requested a review from omangesg February 19, 2025 18:29
@Yury-Fridlyand
Copy link
Collaborator

Hi @rueian,
your contribution is much appreciated!
Could you please add an integration tests which covers that scenario? Please also add a changelog entry in MD format.
Thanks!

@rueian rueian force-pushed the fix-channel-passing branch from 111af00 to 48de519 Compare February 19, 2025 18:39
@rueian rueian changed the title Go: use cgo.Handle to pass channel pointers to Rust and back to Go Go: fix channel passing from Go->Rust->Go by runtime.Pinner Feb 19, 2025
@jamesx-improving
Copy link
Collaborator

Hi @rueian ,

Thank you for your contribution!

Just want to quickly address the CI failure on Go 1.20: it seems like runtime.Pinner was introduced in Go 1.21, hence the 1.20 CI failure. While I like the simplicity of using runtime.Pinner, we've committed to support Go 1.20, so maybe fallback to the cgo.Handle solution you proposed earlier? I'm actually quite new to Go, so I'll leave you to choose the best solution here :)

Thanks!

@rueian rueian force-pushed the fix-channel-passing branch 5 times, most recently from 3911b2a to 6dd79ff Compare February 19, 2025 19:51
@rueian
Go: fix channel passing from Go->Rust->Go by runtime.Pinner
65137d3 
The current way of passing uintptr(unsafe.Pointer) to rust and back
to Go is not safe for dereferencing. The address can still be moved by Go.

Starting from Go 1.21, we can use runtime.Pinner to make an address fixed.
Before Go 1.21, we can use cgo.Handle but it is subject to overflow.
https://cs.opensource.google/go/go/+/refs/tags/go1.24.0:src/runtime/cgo/handle.go;l=111

Signed-off-by: Rueian <[email protected]>
@rueian rueian force-pushed the fix-channel-passing branch from 6dd79ff to 038570d Compare February 19, 2025 20:58
@rueian rueian force-pushed the fix-channel-passing branch from 038570d to 05c52c3 Compare February 19, 2025 21:06
@rueian rueian changed the title Go: fix channel passing from Go->Rust->Go by runtime.Pinner Go: Fix channel passing from Go to Rust by using runtime.Pinner or cgo.Handle Feb 19, 2025
@rueian
Copy link
Author

rueian commented Feb 19, 2025

Hi @Yury-Fridlyand,

The integration test and the changelog entry have been added.

Hi @jamesx-improving,

I have made this PR work for Go 1.20 using the cgo.Handle and for newer versions using the runtime.Pinner by wrapping them into a shared interface.

cgo.Handle itself is a little dangerous to use: it will overflow and panic eventually: https://cs.opensource.google/go/go/+/refs/tags/go1.24.0:src/runtime/cgo/handle.go;l=111

So I would suggest the best option is actually not to support Go 1.20.

Yury-Fridlyand
Yury-Fridlyand reviewed Feb 19, 2025
View reviewed changes
Yury-Fridlyand
Yury-Fridlyand approved these changes Feb 19, 2025
View reviewed changes
Copy link
Collaborator

@Yury-Fridlyand Yury-Fridlyand left a comment

Choose a reason for hiding this comment

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

Thank you so much @rueian

@Yury-Fridlyand Yury-Fridlyand mentioned this pull request Feb 19, 2025
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@omanges omanges omanges left review comments

@Yury-Fridlyand Yury-Fridlyand Yury-Fridlyand approved these changes

@omangesg omangesg Awaiting requested review from omangesg

Assignees
No one assigned
Labels
go golang wrapper
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Go: the client is stuck on sending responses to the wrong channel
4 participants
@rueian @Yury-Fridlyand @jamesx-improving @omanges