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

testing: TB.TempDir paths can be too long for Unix socket paths #62614

Open
bradfitz opened this issue Sep 13, 2023 · 4 comments
Open

testing: TB.TempDir paths can be too long for Unix socket paths #62614

bradfitz opened this issue Sep 13, 2023 · 4 comments
Labels
NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. Unfortunate
Milestone
Backlog

Comments

@bradfitz
Copy link
Contributor

What version of Go are you using (go version)?

Go 1.21
darwin/arm64

What did you do?

I renamed a test to make it more descriptive (longer) and then my test failed:

    integration_test.go:879: sockFile path "/var/folders/0f/7sz95dc94nj46b6yz8p_fqd40000gn/T/TestIncrementalMapUpdatePeersRemoved1080118162/001/tailscale.sock" (len 114) is too long, must be < 104

Turns out darwin, openbsd, and freebsd at least limit the length of a unix socket to 104 bytes.

Because os.TempDir on darwin is a long user-specific thing (e.g. /var/folders/0f/7sz95dc94nj46b6yz8p_fqd40000gn/) and testing.TB.TempDir includes the test name and some suffix (TestIncrementalMapUpdatePeersRemoved1080118162), by the time those are all concatenated, you're out of space for a unix socket filename that fits in 104 bytes.

Apparently @josharian discovered this two years ago in tailscale/tailscale@f80193f where I approved a rename to make test names shorter but it didn't cross my mind to file this bug until I just hit it again now.

I ended up working around it by:

	dir := t.TempDir()
	sockFile := filepath.Join(dir, "tailscale.sock")
	if len(sockFile) >= 104 {
		// Maximum length for a unix socket on darwin. Try something else.
		sockFile = filepath.Join(os.TempDir(), rands.HexString(8)+".sock")
		t.Cleanup(func() { os.Remove(sockFile) })
	}

But that's a little gross.

What did you expect to see?

Operating systems to be less annoying. Failing that, maybe the testing package could help work around it somehow?

What did you see instead?

Failure to make a unix socket in a test.
@apparentlymart
Copy link

FWIW I've also seen similar failures in a non-test context with github.com/hashicorp/go-plugin, which on non-Windows systems uses temporary unix sockets to communicate between plugin and host processes. By default it uses os.MkdirTemp to choose a directory to place those in, which can potentially cause the same situation.

I've not reported it before because I didn't really know what to propose other than your "operating systems to be less annoying" expectation. 😀

I expect it's defensible for the testing library to do weird workarounds just to make things work but less defensible for os.MkdirTemp to do so, and so I'm not meaning to suggest that this issue's scope grow to include the underlying os function, but if a workaround were found and implemented in a place that libraries other than testing could call it then I'd probably propose to make the go-plugin library use it too, since it has a similar set of concerns.

@heschi heschi added the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Sep 13, 2023
@heschi heschi added this to the Backlog milestone Sep 13, 2023
@heschi
Copy link
Contributor

heschi commented Sep 13, 2023

cc @bcmills

@bcmills
Copy link
Contributor

bcmills commented Sep 13, 2023

That's less about TempDir and more about Unix domain sockets, unfortunately. 😅
In general nothing guarantees that even os.TempDir() is short enough to be used with the POSIX socket API. (os.TempDir() on macOS in particular can be quite long.)

In theory nettest.LocalPath can be used for this sort of testing, but looking at its implementation it seems to be racy. 😩

net.testUnixAddr is more robust (because I deflake it to resolve said race), but is unfortunately unexported.

Probably a better replacement for nettest.LocalPath is the best we can hope for?

@bcmills bcmills changed the title testing: TB.TempDir paths can be too long sometimes testing: TB.TempDir paths can be too long for Unix socket paths Sep 13, 2023
@bcmills bcmills added the WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. label Sep 13, 2023
@bradfitz
Copy link
Contributor Author

Ah, I didn't know about nettest.LocalPath. Thanks.

If we wanted to be really (too?) fancy, we could make the error message on failure to open up a long unix socket refer to nettest.LocalPath to educate users as to alternatives? At least if the path contained something tmp- or test-looking, heuristically?

@mayankbh mayankbh mentioned this issue Sep 21, 2023
Merged
13 tasks
@seankhliao seankhliao removed the WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. label Oct 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. Unfortunate
Projects
None yet
Milestone
Backlog
Development

No branches or pull requests
5 participants
@bradfitz @apparentlymart @bcmills @seankhliao @heschi