Extend constructor to accept resource constraints #41

konradreiche · 2019-02-05T16:02:18Z

For a project we're in dire need to limit the runtime's memory used by the VM. For that I've extended the Isolate constructor to accept the resource constraint type.

augustoroman · 2019-04-18T06:25:55Z

This is great, thanks for the contribution! Can you document the new exported type & function, and add some basic lights-on tests? Better yet, add a test that verifies the memory constraints.

konradreiche · 2019-05-02T14:32:44Z

Yes! I just got started and designing the unit test is probably the most interesting part, because if the memory threshold is hit, v8 just crashes with an OOM error message.

You can set a OOMErrorHandler function, and we could probably even interface that with a Go method we can register but it still seems unclear to me how to encapsulate that in a test. Basically, the test should only succeed if the OOMErrorHandler is invoked but we might still end up with a lot of verbose debug messages in the test like this here:

<--- JS stacktrace --->

==== JS stack trace =========================================

    0: ExitFrame [pc: 0x37606b0409d]
    1: StubFrame [pc: 0x37606be16fb]
Security context: 0xc85f8258a1 <JSObject>
    2: /* anonymous */ [test.js:1] [bytecode=0xc85f80ffa9 offset=31](this=0xc85f871691 <JSGlobal Object>)
    3: InternalFrame [pc: 0x37606b1b96f]
    4: EntryFrame [pc: 0x37606bd0ce1]

==== Details ================================================

[0]: ExitFrame [pc: 0x37606b0409d]
[1]: StubFrame [pc: 0x37606be16fb]
[2]: /*...

augustoroman · 2019-05-02T15:59:08Z

Interesting, can you expand on how this new feature works? I was under the impression that if you set the resource constraint and it's exceeded within the JS VM, then any JS execution will fail with some v8 error. What do you mean by "v8 just crashes with an OOM error message" -- do you mean that it crashes the process? or returns an error to go with the OOM? If the latter, then a test that just looks for that error should be sufficient.

konradreiche · 2019-05-02T16:01:21Z

It actually brings down the whole process. The moment the heap is running out of memory, v8 will kill the process with a fatal error. I think v8 starts by default with max_old_memory_space set to 512MB and if you need more, than you can use that parameter to tweak it.

I've prototyped something where you can define your own OOM Error Handler in Go. I'll update my PR shortly.

konradreiche · 2019-05-02T16:18:17Z

The v8 API makes it possible to set your own OOM error handler. This is useful for us, because what we want is to let v8 run into a controlled OOM scenario and then let the test pass. Since this is a Go wrapper we should be able to define this handler in Go and not only C and make it configurable to the client using it.

I've done that by exporting the function OOMErrorHandler and adding an Isolate method AddOOMErrorHandler. This works well given the test I've added. What I am not happy with is the fact that I was only able to implement a global OOM error handler.

I'd rather make it possible to configure the OOM error handler per Isolate, but my cgo knowledge is not sufficient to know how to make this work. When I try to export a Go method as part of a struct it won't work. It looks like I can only export functions but no methods to cgo.

So I have no idea how to scope this per isolate.

augustoroman

Thanks for working on adding the test. Let me know when it's ready for review.

augustoroman · 2019-05-02T16:15:58Z

v8.go

+
+// NewIsolateWithConstraints creates a new V8 Isolate applying additional
+// resource constraints to limit the V8 runtime.
+func NewIsolateWithConstraints(constraints ResourceConstraints) *Isolate {


In retrospect, the NewIsolateWithSnapshot was a mistake -- we should have NewIsolateWithOptions so that every new constructor option doesn't add another constructor.

Yes. Is it too late for that given that the existing API shouldn't be broken?

It's too late to remove NewIsolateWithSnapshot, but we can add NewIsolateWithOptions and stem the bleeding. Add a note that NewIsolateWithSnapshot is deprecated and just forward the implementation to a call to ...WithOptions

augustoroman · 2019-05-02T16:17:58Z

v8.go

+
+type ResourceConstraints struct {
+	// MaxOldSpaceSize sets the maximum size of the old object heap in MiB.
+	MaxOldSpaceSize int


Please doc the behavior when the resource limits are hit. That it crashed the process was a surprise! I'm happy that you're adding the OOM handler.

augustoroman · 2019-05-02T16:18:39Z

v8_test.go

+	isolate := NewIsolateWithConstraints(ResourceConstraints{MaxOldSpaceSize: 10})
+	isolate.SetOOMErrorHandler(func(location string, isHeapOOM bool) {
+		// Success
+		os.Exit(0)


This is not ok to do in a test since it'll interrupt other tests. If any other tests are failing, you might not see that because they are run in parallel but this one exits the entire test process before anything else fails.

Glad you mentioned this. I assumed the same but didn't see the other tests stopping but didn't think of the implications of parallel test execution!

I am wondering if there's another way. Simply, without os.Exit(0) the test will fail because of the v8 runtime sending a non-0 exit code.

There's also a SetFatalErrorHandler we could set for v8, maye that would allow to override the behavior for the test scenario.

If you need to test that the process crashes, then you can have this test run a separate test process:

https://stackoverflow.com/questions/26225513/how-to-test-os-exit-scenarios-in-go

(though I'd prefer that we end up with a cleaner situation than crashing the process -- if you end up with a crash-the-process solution, please be VERY VERY clear in the docs about the side-effects!)

augustoroman · 2019-05-02T16:19:18Z

v8_test.go

@@ -1541,3 +1542,23 @@ func TestPanicHandling(t *testing.T) {
 	_ = NewIsolate()
 	_ = *f
 }
+
+func TestNewIsolateWithConstraints(t *testing.T) {
+	// Creates a v8 runtime where the memory is limited to 10MB and memory is


no need to set it as high as 10MB -- 1MB is fine. Tests should be as lightweight as possible.

With 1MB it seems NewIsolate() doesn't even terminate. I think there's a minimum required memory to get v8 started. This should probably be also tested as part of the configuration input, i.e. illegal parameters.

augustoroman · 2019-05-02T16:37:07Z

I'd rather make it possible to configure the OOM error handler per Isolate, but my cgo knowledge is not sufficient to know how to make this work. When I try to export a Go method as part of a struct it won't work. It looks like I can only export functions but no methods to cgo.

This is indeed the case. We fight with the same thing when handling the callbacks:
https://github.com/augustoroman/v8/blob/master/v8.go#L501-L521

I think you should be able to piggyback on that code (or, if not, use the same approach). I'd like to help but I won't have time in the next few weeks to allocate to this, sorry.

konradreiche · 2019-05-02T16:38:09Z

Awesome! No worries, a blueprint is all I need. I'll get to it :)

augustoroman · 2019-05-07T19:26:20Z

Yeah, I think that should work.

…

On Tue, May 7, 2019 at 8:19 AM Konrad Reiche ***@***.***> wrote: I just read through the code of callback magic. Since this implementation is scoped to the v8.Context object and adding OOM error handlers is scoped to v8.Isolate should I go ahead and create another registry, i.e.: var contexts = map[int]*refCount{} var contextsMutex sync.RWMutex var nextContextId int var isolates = map[int]*refCount{} var isolatesMutex sync.RWMutex var nextIsolateId int ? — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#41 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAFFFVM6VKWPOSBMUUM7ZI3PUGFYPANCNFSM4GUODUAA> .

konradreiche · 2019-05-14T15:31:09Z

Coming back to this after picking my brain for a while on this topics. Here are my thoughts:

I assumed setting an OOM error handler per isolate would make sense since this is what the v8 API exposes. After implementing everything I realized when an OOM error happens, there is no way to recover from it:

recover() won't work because v8 will invoke a FATAL and thus os::abort, there is now way to recover from that, also in general, common opinion seems to be, don't try to recover from an OOM to begin with, but shutdown gracefully.

So it still stands, that we want to make that happen in Go code. So I came back to my initial implementation, set an OOM error handler globally with v8.SetOOMErrorHandler. The way I implemented it, ensures that old isolates as well as new isolates will get an updated version of the OOM error handler whenever the function is called.

I've also changed the code to have a NewIsolateWithOptions(opts IsolateOptions) (*Isolate, error) constructor which takes care of setting a snapshot, as well as resource constraints. I've added an error return value, too, in order to catch situations where users are setting the max memory old space size too little for the v8 to initialize in the first place.

To reiterate on some of your thoughts @augustoroman. My code changes don't introduce any new situations where the process can come down crashing. With the code currently on master, the same thing can happen when you create a new isolate and keep on allocating memory. When you allocate more than 512MB of memory, v8 will crash, and thus the Go process.

Obviously, it's a lot harder to get OOM with 512MB than with 10MB so extending the documentation to point out the importance is still something I need to do.

What do you think of this so far?

I've also wrote to the v8-dev mailing list for clarification on the OOM error handling and why it's per isolate so we don't make any mistakes here https://groups.google.com/forum/#!topic/v8-dev/fvw21ElZlSY

Extend constructor to accept resource constraints

b41b206

konradreiche force-pushed the new/memory-constraint branch from 6ed3035 to 1173852 Compare May 2, 2019 16:14

augustoroman requested changes May 2, 2019

View reviewed changes

Implement v8 OOM error handling in Go

d7ada8c

konradreiche force-pushed the new/memory-constraint branch from 1173852 to d7ada8c Compare May 14, 2019 15:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Extend constructor to accept resource constraints #41

Extend constructor to accept resource constraints #41

konradreiche commented Feb 5, 2019

augustoroman commented Apr 18, 2019

konradreiche commented May 2, 2019

augustoroman commented May 2, 2019

konradreiche commented May 2, 2019

konradreiche commented May 2, 2019

augustoroman left a comment

augustoroman May 2, 2019

konradreiche May 2, 2019

augustoroman May 2, 2019

augustoroman May 2, 2019

augustoroman May 2, 2019

konradreiche May 2, 2019

augustoroman May 2, 2019

augustoroman May 2, 2019

augustoroman May 2, 2019

konradreiche May 2, 2019

augustoroman commented May 2, 2019

konradreiche commented May 2, 2019

augustoroman commented May 7, 2019 via email

konradreiche commented May 14, 2019

Extend constructor to accept resource constraints #41

Are you sure you want to change the base?

Extend constructor to accept resource constraints #41

Conversation

konradreiche commented Feb 5, 2019

augustoroman commented Apr 18, 2019

konradreiche commented May 2, 2019

augustoroman commented May 2, 2019

konradreiche commented May 2, 2019

konradreiche commented May 2, 2019

augustoroman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

augustoroman commented May 2, 2019

konradreiche commented May 2, 2019

augustoroman commented May 7, 2019 via email

konradreiche commented May 14, 2019