Skip to content

Conversation

londek
Copy link

@londek londek commented Apr 9, 2025

Hey,
this implements IsEmpty check.
It tells developer if context was Reset, freed or what you call here in v8go - closed.
It's useful for checking during callbacks whether context is still alive - so whether JS callback function still lives.
Note: It's a direct dependency of my upcoming PR in https://github.com/kuoruan/v8go-polyfills
Thanks for this brilliant fork, please consider making it a separate repo ;)

@tommie
Copy link
Owner

tommie commented Apr 9, 2025

Hi. Thanks for the consideration.

It doesn't sound good that you get callbacks after the context has been closed. Since closing the context frees tracked values, this feels like it could lead to undefined behavior.

I think adding this function may make it feel safe to not care about the Context lifetime, which is absolutely not the case.

@londek
Copy link
Author

londek commented Apr 9, 2025

Yes, context lifetimes are a valid and serious point.

Solution could be implementation of event loop, entire event loop system just to run setTimeout. Let's be serious about this for a minute. If it takes this much effort you might as well just setup IPC between Go and Node instance. Big v8go advantage is that it's minimalistic, it's lightweight and it just (sometimes) works. I can't see how timers could nowadays virtually be unavailable on even most lightweight JS runtimes (actually, googling, even qjs has out of the box timeout support)

Please consider function setTimeout(fn, delay) as example.
setTimeout would in simple words execute Go function which would create goroutine.
That goroutine would wait for time.After(delay) and then call fn.Call(...).
From the point of setTimeout's Go implementation it's not known if context is still alive by the time timeout is reached. It just cannot know, because ctx.Close() might happen anytime and there is no way to check it currently.

You've mentioned that closing frees tracked values. What about introducing finalizers for ContextPtr (or even go-world Context). That way developer could call ctx.AddFinalizer(t.close) and thereby stop (and cleanup!) timers.
Identifier AddFinalizer could feel ambiguous as runtime.SetFinalizer already exists and has strong Go's GC meaning in gophers mentality.

One other way could be adopting Done() <-chan struct{}, part of context.Context to v8go context which I'm sceptical of.

@stroiman
Copy link

stroiman commented Apr 10, 2025

Solution could be implementation of event loop, entire event loop system just to run setTimeout

I think there's something that should be made very clear, v8go is a Go adapter of the V8 engine, which is itself an ECMAScript execution engine. setTimeout and friends are not part of the ECMAScript specifications, and therefore doesn't exist in V8 and shouldn't exist in v8go either.

V8 itself doesn't run concurrently, and will segfault if client code tries to do so in the same Isolate.

So while I'm not sure what problem you're trying to solve, it appears to be something you need to control on the caller side. And if you want to add an event loop on top, you need to deal with synchronizing shutdown in client code.

E.g., in a browser, the event loop runs in the main thread, so JS code can never run concurrently with any DOM modifications triggered by user interaction; including shutdown.

@tommie
Copy link
Owner

tommie commented Apr 10, 2025

Thanks for the explanation.

Since the only way to free a context is for the caller to issue Context.Close, a solution would be to for the caller to stop the timer goroutines before closing the context. I.e. a ContextWithTimers wrapper. This is possible with the existing API, unless I'm missing something.

(Side-note: v8 Isolate is thread-compatible, but not thread-safe, so make sure there is locking in the timer goroutines.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants