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.

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

net: TestSplice/big can lead to "out of memory" panics #26867

Closed
mvdan opened this issue Aug 8, 2018 · 5 comments

Comments

Projects
None yet
3 participants
@mvdan
Copy link
Member

commented Aug 8, 2018

$ go version
go version devel +9ef5ee911c Tue Aug 7 14:36:14 2018 +0000 linux/amd64
$ free -m
              total        used        free      shared  buff/cache   available
Mem:           7671        2308        3929         939        1433        4185
$  go test net
signal: killed
FAIL    net     9.743s

If I do go test -v net, I can see:

[...]
=== RUN   TestTCPSelfConnect
fatal error: runtime: out of memory
[...]

I presume that TestSplice/big is the culprit, as it seems to be the test that allocates the most memory by far. I realise that it's the purpose of the test, but I still think this test shouldn't fail if one doesn't have enough memory available.

I realise that in this scenario I only have about 4GB of available memory, but that still seems to me like it should be plenty to be able to run go test std. In fact, I encountered this crash while running that command with about 6GB of available memory (to reproduce with just go test net, I started using more memory to save time).

I wonder what would be the best way to keep the test working as it is on systems with more memory, while keeping it from crashing the runtime on systems with less memory. Even if we had a way to check the available memory at the start of the test, that still isn't a guarantee that we won't crash the runtime.

There's always the option of saying "at least 8GB of available memory is required to run go test std". But hopefully that required minimum would be much lower - 8GB of physical memory is still common, even in fairly new laptops out there.

@mvdan

This comment has been minimized.

Copy link
Member Author

commented Aug 8, 2018

cc @mikioh @bradfitz @ianlancetaylor as per the owners doc. Also cc @acln0, who I believe added the code and tests in question.

@acln0

This comment has been minimized.

Copy link
Contributor

commented Aug 8, 2018

Sorry for the trouble. Perhaps 1 << 31 was excessive indeed. I think any size strictly bigger than 4 << 20 should be fine, since we have this:

// maxSpliceSize is the maximum amount of data Splice asks
// the kernel to move in a single call to splice(2).
maxSpliceSize = 4 << 20
. The purpose of TestSplice/big is just to check that things work correctly if poll.Splice needs to call splice(2) more than once. With this change, perhaps the testing.Short() condition, that makes the size of the test smaller, can also be dropped. It's probably not very useful after all.

@acln0

This comment has been minimized.

Copy link
Contributor

commented Aug 8, 2018

I would have sent a CL with the fix myself, but I'm currently on vacation, and I don't have my work machine with me. Sorry.

@mvdan

This comment has been minimized.

Copy link
Member Author

commented Aug 8, 2018

No worries; thanks for the quick reply! I'll send a CL after lunch :)

@gopherbot

This comment has been minimized.

Copy link

commented Aug 8, 2018

Change https://golang.org/cl/128535 mentions this issue: net: reduce TestSplice/big's memory usage

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.