BAAS-34801: add forced mem checks on new arrays and obj props #127
Conversation
builtin_array.go
Outdated
| panic(err) | ||
| } | ||
| if memCtx.MemUsageLimitExceeded(memUsage) { | ||
| panic("memory limit exceeded") |
There was a problem hiding this comment.
[q] would the panic know the func name when it happens? We can use the runtime's vm's func name if the stack trace doesn't help us here. I guess we would flag certain apps, so in theory we know which functions are causing the issue. This may help if there are multiple triggers and functions for flagged apps or if we have multiple apps flagged running at the same time.
There was a problem hiding this comment.
I need to see how I can bubble this up to baas, I still haven't ironed out how this comes through in baas so that we can parse it and potentially log more details.
There was a problem hiding this comment.
yea, i agree reporting to baas could be a problem, e.g. how do we report if recursive array objects, where another array embedded into an array, do we only report max memUsage?
There was a problem hiding this comment.
Will delegate to the baas PR to figure this out. But in this case the panic will stop execution and for recursive data structures the poller eventually will catch it. Either way I'll have to test this out and report back. I'll leave this thread open until I have done more tests locally.
There was a problem hiding this comment.
I was able to think about this a little more, we technically use the regular poller to assess mem usage of a function and bill accordingly and I think we should continue to do so. This is just to allow us to save our pods from OOMing with scenario we can't catch in time.
object.go
Outdated
| panic(err) | ||
| } | ||
| if memCtx.MemUsageLimitExceeded(memUsage) { | ||
| panic("memory limit exceeded") |
There was a problem hiding this comment.
same q here about panicking with the func name.
There was a problem hiding this comment.
We don't have access to the function name here where we panic. I could try and see if we can pass it down from baas otherwise it might be cleaner to pick this up from baas when we catch this panic. Will leave this open until I have a definitive answer.
There was a problem hiding this comment.
In goja I def don't have access to the function name, I think we could potentially figure out the currently executing function within the App Services function but I'd rather investigate that later.
|
This https://github.com/10gen/baas/pull/15863 is what the corresponding PR looks like. |
| @@ -548,6 +548,17 @@ func (o *baseObject) setOwnStr(name unistring.String, val Value, throw bool) boo | |||
| o.val.runtime.typeErrorResult(throw, "Cannot add property %s, object is not extensible", name) | |||
There was a problem hiding this comment.
[qq] I see that you added a nil check below for o.val, do we need it here as well?
There was a problem hiding this comment.
I was being overly cautious, I won't make changes here since this is from original goja. Thanks for checking though!
This PR attempts to add a forced mem check on paths that have been historically causing OOMs. The poller is unable to catch these OOMs so we attempt to force a mem check ad-hoc where we usually risk very high memory usage.
For what it's worth I tried a small benchmark with this change