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

runtime: go can allocate 30x the memory limit without panic'ing #68934

Closed
ldemailly opened this issue Aug 18, 2024 · 10 comments
Closed

runtime: go can allocate 30x the memory limit without panic'ing #68934

ldemailly opened this issue Aug 18, 2024 · 10 comments
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone

Comments

@ldemailly
Copy link

ldemailly commented Aug 18, 2024

Go version

1.22.6 up to gotip (and playground)

What did you do?

https://go.dev/play/p/Bid4PkG5o7e

package main
import (
	"fmt"
	"runtime/debug"
	"strings"
)
func main() {
	lim := 2_000_000 // less than 2MiB
	debug.SetMemoryLimit(int64(lim))
	v := strings.Repeat("ABC", 10*lim) // 30x the limit
	fmt.Println(len(v))
}

What did you see happen?

No panic, prints
60000000

What did you expect to see?

panic

ps: I am aware GOMEMLIMIT is a soft limit, yet letting it go 30x above in a single alloc isn't a useful feature

I recommend in "large alloc" runtime.(*mcache).allocLarge() to check and panic instead of going ahead.

Here is what I had to do:
https://github.com/grol-io/grol/pull/159/files#diff-f75830b8bf11850b6726e69a5d72e2ac711fca76f0cc5bd457f4ffed15e397de
meanwhile

ps1: it can also fatal error: runtime: out of memory https://gist.github.com/ldemailly/915e043323fa160562f121766990d9e3 if you push it above the OS limit but that's also not helpful as it terminates the program instead of failing the 1 bad alloc (and so the rest of the goroutines also end without a chance for cleanup)

ps2: numbers is the sample are tiny so it runs on the go playground but you can use GiB and get the same outcomes (either it allocates many times over the limit without breaking a sweat, or get OOM killed by the OS or gets the fatal error depending on the numbers) - of note that calling runtime.GC() before/after/inbetween doesn't help
https://github.com/ldemailly/go-scratch/blob/main/membug/membug.go has more stuff I tried to make it panic

@gabyhelp
Copy link

Related Issues and Documentation

(Emoji vote if this was helpful or unhelpful; more detailed feedback welcome in this discussion.)

@prattmic
Copy link
Member

cc @golang/runtime @mknyszek

@prattmic prattmic changed the title go can allocate 30x the memory limit without panic'ing runtime: go can allocate 30x the memory limit without panic'ing Aug 18, 2024
@gopherbot gopherbot added the compiler/runtime Issues related to the Go compiler and/or runtime. label Aug 18, 2024
@ldemailly
Copy link
Author

ldemailly commented Aug 18, 2024

As I know there are objections against panic mid alloc, I think making it and opt-in would be a good compromise
Even though I think the objections are not founded, ie this was mentioned:

mu.lock()
allocSomething()
mu.Unlock()

I think it's fine it can panic, and deadlock if defer isn't not used, that may be better than getting killed;
also allocSomething() could be array[outOfBounds] instead and that can panic today on a similar "mistake" (wrong argument)

Slow death spiral is another better one, but not what we're after here which is to prevent big allocs (ie > 4k) going over

@mknyszek
Copy link
Contributor

At a baseline, I don't think changing GOMEMLIMIT's behavior is possible now, due to the Go 1 backwards compatibility policy. Introducing a new panic is not backwards-compatible.

I also think we are unlikely to add a new API for this. I see you're aware of #63338, but I don't see how this is different from what #63338 is asking for.

As I mentioned in #63338, if you really want a program to die when some memory usage threshold is met, you can already do that by having the program inspect its own runtime state via the runtime/metrics package or runtime.ReadMemStats in a background goroutine. (The memory that GOMEMLIMIT cares about, and how to measure it, is documented in the API docs for runtime/debug.SetMemoryLimit.) It won't die at the very moment the memory limit is exceeded, but if it dies within 100ms (or some other reasonable bound), is that good enough for your use-case? In other words, what about the behavior you want means it should be part of the runtime?

Lastly, I'm not sure it meets the very high bar for a new GC knob/parameter. We would need a lot of evidence and a strong class of use-cases to meet that bar. Crashing is just such a strong response to any situation -- it generally means the program really has no way to progress. But if you set a 30x hard limit and you run on a system with 30x memory, your program could have progressed.

@prattmic
Copy link
Member

I'm not convinced this does much about death spiral risk. If you are within 4kb of GOMEMLIMIT and every request you receive makes a >4kb allocation, then every request will panic. Then you'll recover and "continue" but never make progress.

I think this would be more amenable if we did an unrecoverable panic (a throw), but it sounds like you specifically want to recover?

@ldemailly
Copy link
Author

Just to note that unlike 63338 I don’t want the whole program to die, oom killer/the OS does that, I want the bogus allocations to fail without having to change every single make() append() Repeat() call so the program survives

@cherrymui cherrymui added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Aug 19, 2024
@cherrymui cherrymui added this to the Backlog milestone Aug 19, 2024
@mknyszek
Copy link
Contributor

I want the bogus allocations to fail without having to change every single make() append() Repeat() call so the program survives

Apologies for the misunderstanding; I'm coming from a mindset that allocation-related panics are always unrecoverable.

But that being said, I'm not sure I feel great about the prospect of opening the door to making panics from allocation recoverable. The general case seems unintuitive and not very useful, similar to how malloc (from libc) can fail. What exactly are you supposed to do if malloc fails? Generally speaking, the program cannot proceed.

I understand you're only interested in "bogus" allocations, but such allocations seem like bugs to me in the first place. Re-reading your original post, what is your use-case, more concretely? Under what circumstance do you want to do this over, say, checking to make sure, for example, that allocations proportional to user input are validated properly? For example, if your goal is debugging, maybe we can offer better diagnostics instead.

@ldemailly
Copy link
Author

malloc failing is perfectly fine, you know you can't get that much so you don't (ie you propagate an error and don't necessarily crash, unless crashing is what you want or the only thing you can do, which is also fine too, it becomes your choice)

my use case concretely is I get user input that ends up making allocations, and I want these allocations to not be hardcoded to some arbitrary limit but use the available configured memory of the hosting environment instead, and fail if asking too much (see the links in original description for how I deal with it currently, calling runtime.ReadMemStats(&memStats) and comparing HeapAlloc with debug.SetMemoryLimit(-1) - but while that work for my very controlled environment (I know where all allocs are pretty much) it wouldn't work in general (for instance for servers serving http requests) and requires quite a lot of code changes - runtime.(*mcache).allocLarge() I believe has that info more readily available I think and more cheaply

ps: thanks a lot for your trying to understand my issue and not just dismissing it, appreciated!

@ianlancetaylor
Copy link
Member

We decided long ago that Go programs would not be able to recover from a failure to allocate memory using ordinary Go code like make or new or taking the address of a variable where the address escapes. For some other issues on this general topic see #5049, #14162, #16843, #33060.

@ianlancetaylor
Copy link
Member

This isn't something we are going to do at this point.

@ianlancetaylor ianlancetaylor closed this as not planned Won't fix, can't repro, duplicate, stale Aug 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
Development

No branches or pull requests

7 participants