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
Closed

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

mvdan opened this issue Aug 8, 2018 · 5 comments
Labels
FrozenDueToAge Testing An issue that has been verified to require only test changes, not just a test failure.

Comments

@mvdan
Copy link
Member

mvdan 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 mvdan added Testing An issue that has been verified to require only test changes, not just a test failure. NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Aug 8, 2018
@mvdan
Copy link
Member Author

mvdan 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
Copy link
Contributor

acln0 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
Copy link
Contributor

acln0 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
Copy link
Member Author

mvdan commented Aug 8, 2018

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

@mvdan mvdan removed the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Aug 8, 2018
@gopherbot
Copy link

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

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge Testing An issue that has been verified to require only test changes, not just a test failure.
Projects
None yet
Development

No branches or pull requests

3 participants