.,ruby: enable accept to be called as TsnetAccept & fix gem#24
.,ruby: enable accept to be called as TsnetAccept & fix gem#24
Conversation
This fixes calling accept on the updates listen socket strategy by embedding the accept behavior into the Go exported library, avoiding the mandatory requirement for building a separate object from the C library.
There was a problem hiding this comment.
Pull request overview
This PR moves the listener accept FD-receive logic from the Ruby/C side into the Go-exported library API (TsnetAccept), so language bindings can call accept directly without relying on a separate C-side recvmsg implementation.
Changes:
- Added Go-exported
TsnetAcceptto receive an accepted connection FD viarecvmsg/SCM_RIGHTS. - Simplified the C wrapper
tailscale_acceptto delegate toTsnetAccept. - Updated the Ruby FFI binding and tests to use the new accept path and added a basic listen/accept/dial roundtrip test.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
tailscale.go |
Adds exported TsnetAccept and imports x/sys/unix for control-message sizing. |
tailscale.c |
Replaces C-side recvmsg accept logic with a call into TsnetAccept. |
ruby/lib/tailscale.rb |
Switches Ruby binding to TsnetAccept and changes listener accept/close behavior. |
ruby/test/tailscale/test_tailscale.rb |
Adds a new accept/dial roundtrip test and a helper to wait for backend readiness. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| buf := make([]byte, unix.CmsgLen(int(unsafe.Sizeof((C.int)(0))))) | ||
| _, oobn, _, _, err := syscall.Recvmsg(int(listenerFd), nil, buf, 0) | ||
| if err != nil { | ||
| return ln.s.recErr(err) | ||
| } | ||
|
|
||
| scms, err := syscall.ParseSocketControlMessage(buf[:oobn]) | ||
| if err != nil { | ||
| return ln.s.recErr(err) | ||
| } | ||
| if len(scms) != 1 { | ||
| return ln.s.recErr(fmt.Errorf("libtailscale: got %d control messages, want 1", len(scms))) | ||
| } | ||
| fds, err := syscall.ParseUnixRights(&scms[0]) |
There was a problem hiding this comment.
The control-message buffer is sized with unix.CmsgLen, which is the length value for a cmsghdr, not the space needed for receiving ancillary data. This can lead to truncated SCM_RIGHTS data and failed accepts on some platforms. Allocate the OOB buffer with unix.CmsgSpace(sizeof(int)) (or syscall.CmsgSpace) and check recvmsg flags for MSG_CTRUNC/MSG_TRUNC before parsing.
| lio = IO.for_fd(@listener) | ||
| until IO.select([lio]).first.any? | ||
| @ts.assert_open |
There was a problem hiding this comment.
IO.for_fd defaults to autoclose=true; the temporary lio created here can be garbage-collected after accept returns, which would close the underlying listener FD unexpectedly. Use IO.for_fd(@Listener, autoclose: false) (or store a persistent IO wrapper on the Listener instance and close it in #close) so GC can’t close the listener.
| until IO.select([lio]).first.any? | ||
| @ts.assert_open |
There was a problem hiding this comment.
This until IO.select([lio]).first.any? loop is effectively a single blocking call because IO.select has no timeout; the body won’t re-run to re-check assert_open. Either pass a small timeout to IO.select and loop, or remove the loop and just do a single select (or go back to a fully blocking accept call) to avoid misleading control flow.
| until IO.select([lio]).first.any? | |
| @ts.assert_open | |
| loop do | |
| # Wait briefly for the listener to become readable so we can | |
| # periodically re-check that the server is still open. | |
| readable, = IO.select([lio], nil, nil, 0.1) | |
| @ts.assert_open | |
| break if readable && !readable.empty? |
| end | ||
|
|
||
| def wait_status_running ts | ||
| while ts.local_api.status["BackendState"] != "Running" |
There was a problem hiding this comment.
wait_status_running can loop forever if the backend never reaches "Running", which can hang the test suite. Add a timeout/deadline (and possibly a short backoff) and fail the test with a helpful message when exceeded.
| while ts.local_api.status["BackendState"] != "Running" | |
| timeout_seconds = 30 | |
| deadline = Time.now + timeout_seconds | |
| loop do | |
| status = ts.local_api.status | |
| return if status["BackendState"] == "Running" | |
| if Time.now > deadline | |
| flunk "BackendState did not reach 'Running' within #{timeout_seconds} seconds. Last status: #{status.inspect}" | |
| end |
| def test_listen_accept_dial_close | ||
| ts = newts | ||
| ts.up | ||
| wait_status_running ts | ||
| hn = ts.local_api.status["Self"]["TailscaleIPs"][0] | ||
| s = ts.listen "tcp", "#{hn}:1999" | ||
| c = ts.dial "tcp", "#{hn}:1999" | ||
| ss = s.accept | ||
| c.write "hello" | ||
| assert_equal "hello", ss.read(5) | ||
| ss.write "world" | ||
| assert_equal "world", c.read(5) | ||
| ss.close | ||
| c.close | ||
| ts.close | ||
| end |
There was a problem hiding this comment.
Indentation in this new test method is inconsistent with the rest of the file (2-space indentation). Reformat the body to match the surrounding style to keep the test file readable.
1 participant
This fixes calling accept on the updates listen socket strategy by embedding the accept behavior into the Go exported library, avoiding the mandatory requirement for building a separate object from the C library.