Add Isolate.SetPromiseRejectedCallback

[stroiman]

Allows the embedder to be notified when

• A promise rejects without a handler
• A handler is attached to a rejected promise
• A handler is resolved (fulfilled or settled) more than once

In the process I added a small helper, as these 7 lines appear everywhere

  m_value* new_val = new m_value;
  new_val->id = 0;
  new_val->iso = iso;
  new_val->ctx = ctx;
  new_val->ptr = Global<Value>(iso, result);
  rtn.value = tracked_value(ctx, new_val);
  return rtn;

Btw, On the Go side, Value has a ctx *Context - I don't think it's used - will check if it can be removed.

[stroiman]

There's apparently some confusing naming as "resolve" means the promise is successful (this is what I always thought of "resolve" means). It's also called "fulfilled".

But "resolved" means it has reached it's final state, also called settled. So a "rejected" promise is also "resolved" - which I found confusing.

I only ever used the term "settled" for the latter, so I had a bit of trouble understanding the enum values (lacking documentation in the V8 sources)

[tommie]

If there was already a callback, this should deallocate the handle used by that callback. (which means I'm skeptical of the generic handles implementation.)

[stroiman]

Good catch - didn't think about this as I'd only set it up once.

Can you elaborate on the skeptical part?

[stroiman]

On a side note, The library has (at least) two Go error implementations. JSError and Exception.

*Exception represents a JavaScript Error object (at least "native errors" - don't know if class MyErr extends Error{} counts)

JSError is generated by v8go when a script invocation fails - but it doesn't include the thrown value.

In this particular case, the value is the *Exception value - so I can retrieve all the properties on the actual error object.

JSError does have some useful properties, e.g. for JavaScript code like throw "I'm a string but should have been an error object" - as you now have the stack trace - but the original error is missing, so if the error is returned back to JS - the original error value is lost. E.g., a function callback that itself calls back into JS - which fails with an error.

I might take a look at that eventually - but it's not a priority for my use cast ATM - just wanted to mention this.

[tommie]

Can you elaborate on the skeptical part?

I think SetPromiseRejectedCallback should "own" its handle, which suggests the handle should be stored in a promiseRejectedCallback field, and not be mixed in with whatever else might end up in .handles. Right now, it's a philosophical question, since there's only one user of that field.

On a side note, The library has (at least) two Go error implementations. JSError and Exception.

I added Exception (rogchap#195) because there was no way to for functions to signal failure to Go code. It's one of the reasons for this fork. That created a discussion in rogchap#274, which stalled. Merging them would indeed be nice, and it sounds like I created a new type to avoid modifying too much of the code base without owner blessing.

[stroiman]

I think SetPromiseRejectedCallback should "own" its handle

Ah, I think you're right, but maybe the promiseRejectedCallback shouldn't even have a handle. As I assume that only one callback can be set, it could just be a field on the Go Isolate? And then optionally a handle to the isolate?

type Isolate struct {
  selfHandle cgo.Handle
  promiseRejectedCallback RejectedPromiseCallback
}

//export goCallback
func goCallback(C.uintptr_t handle, /* ... reset */) {
  iso, ok := cgo.Handle(handle).Value().(*Isolate)
  iso.promiseRejectedCalback( /* ... */ );
}

func (i *Isolate) SetPromiseRejectedCallback(cb RejectedPromiseCallback) {
  if i.selfHandle == 0 { 
    i.selfHandle = cgo.NewHandle(i)
  }
  // This could potentially be moved _inside_ the if statement - as we just set the
  // same callback with the same handle. But could be dangerous if the handle is
  // used for different purposes.
  C.IsolateSetPromiseRejectedCallback(C.uintptrt(i.selfHandle), /* ... */ )
  ...
}

func (i *Isolate) Dispose() {
  if i.selfHandle != 0 { 
    i.selfHandle.Delete()
  }
}

[stroiman]

I added Exception

Nice 💪 I'm so glad you did :) I also experimented with Goja to have a pure Go alternative - and it uses panics to return with errors, e.g., you need to panic(NewTypeError(...)) or something like that - but it also looses track of the error value. It feels very wrong.

The stalled discussion was titled, "Store error object in JSError for propagation". I would probably have suggested just to add a *Value field to JSError - as it's not only objects you can throw, e.g. throw 1 is valid, but poor JS. You might even make it an anonymous/embedded field, so all Value methods are promoted. I.e., it's just a value, but has additional information about error site.

[stroiman]

A side note on the side note. Exception values don't work with AsObject(), as it checks IsObject() - and that returns false for functions, exceptions, and module namespace objects.

In my ESM experimental branch, I have

func (v *Value) AsObject() (*Object, error) {
	if !v.IsObject() && !v.IsModuleNamespaceObject() && !v.IsFunction() && !v.IsNativeError() {
		return nil, errors.New("v8go: value is not an Object")
	}
	return &Object{v}, nil
}

[stroiman]

Ok, just checked the readme - I see you're also to thank for Symbol support 🙌 without which I wouldn't have proper iterables in my browser

[tommie]

Rename handle to cb to link it to SetPromiseRejectedCallback(cb).

[tommie]

Good idea to make a helper.

Shouldn't this be an m_value constructor (without tracked_value())? Seems RAII-worthy.

[stroiman]

I have had similar ideas, but something about cleanup I didn't think regarding freeing up of resources.

The Context has a list of values to free, but that's an unordered_map<long, m_value*> - which is released in

void ContextFree(ContextPtr ctx) {
  // ...
  for (auto it = ctx->vals.begin(); it != ctx->vals.end(); ++it) {
    auto value = it->second;
    value->ptr.Reset();
    delete value;
  }
  // ...
  delete ctx;
}

If this had been unordered_map<long, m_value> - that iteration shouldn't be necessary at all AFAICT - as that happens automatically in delete ctx

Btw, I think that there's some mechanism in Go - which is also used some places in the code to cleanup on GC - but not all places. So a lot of temporary Values are never freed until you delete the context. It's not a big problem for my intended use case as contexts are short-lived - but it's something I've wanted to look into.

[stroiman]

On a related note - in a different branch, I added helper functions to the struct, also to simplify commonly occurring patterns.

I'm hoping I'm not making a huge mistake - that a value could suddenly be used in a different isolate than the one that created it? If so, the iso should be an argument - and probably not present as a field in the struct.

struct m_value {
  long id;
   v8::Isolate* iso;
   m_ctx* ctx;
   v8::Global<v8::Value> ptr;
   v8::Local<v8::Value> ToLocal() { return this->ptr.Get(this->iso); }
 };

[stroiman]

And, what's with the m_ prefix? Is it a C/C++ convention?

[tommie]

I'm hoping I'm not making a huge mistake - that a value could suddenly be used in a different isolate than the one that created it? If so, the iso should be an argument - and probably not present as a field in the struct.

While Value itself doesn't seem tied to an Isolate, all concrete constructors (well New()) seem to take an Isolate. So I think we're good with that assumption.

m_ is used extensively in C++ standard libraries for member data, so it does feel weird to me too to have it on type names. And I can't find that it would be a libv8 thing either.

[stroiman]

Ah yes - remember the m_ prefix from ATL I think (the MS lib to build COM objects). I adopted when I first started coding .NET 1.0 😆

[tommie]

go vet is complaining that this is possible misuse of unsafe.Pointer, probably because of the uintptr cast.

[stroiman]

Yeah - honestly, I am totally confused about the relation between C's uintptr_t and void* - maybe you know? (I have a strong suspicion your C/C++ knowledge far exceeds mine)

As far as I could tell, conversion between the two should be safe, and for passing cgo.Handle values in the ExternalValue branch, I passed the uintptr_t, and converts to/from void* in C++.

Any thoughts?

[tommie]

uintptr_t is an unsigned integer type as least as large as the platform's pointers. There's also intptr_t and ptrdiff_t for relative pointers. I.e. very similar in scope of size_t and ssize_t...

• https://rgambord.github.io/c99-doc/sections/7/18/1/4/index.html
• https://rgambord.github.io/c99-doc/sections/7/17/index.html

Seems you shouldn't use unsafe.Pointer with Handle, and instead do the conversion to pointer on the C-side: https://pkg.go.dev/runtime/[email protected]#Handle

[stroiman]

I'd love to complete this, I'm a little in doubt what would be best - what data to store as "data" for the handler. The Go callback, or just a handle to the Isolate?

I was a little uncertain what your suggestion was - but I sketched an alternate solution, where a handle to the isolate is used, rather than a handle to the callback - as the function signature strongly indicates you can have only one callback registered at a time. (One thing I didn't check is if you can set it to null to clear the handle - I think I'll add that too)

type Isolate struct {
  selfHandle cgo.Handle
  promiseRejectedCallback RejectedPromiseCallback
}

//export goCallback
func goCallback(C.uintptr_t handle, /* ... reset */) {
  iso, ok := cgo.Handle(handle).Value().(*Isolate)
  iso.promiseRejectedCalback( /* ... */ );
}

func (i *Isolate) SetPromiseRejectedCallback(cb RejectedPromiseCallback) {
  if i.selfHandle == 0 { 
    i.selfHandle = cgo.NewHandle(i)
  }
  // This could potentially be moved _inside_ the if statement - as we just set the
  // same callback with the same handle. But could be dangerous if the handle is
  // used for different purposes.
  C.IsolateSetPromiseRejectedCallback(C.uintptrt(i.selfHandle), /* ... */ )
  ...
}

func (i *Isolate) Dispose() {
  if i.selfHandle != 0 { 
    i.selfHandle.Delete()
  }
}```

You mentioned renaming `handle` to `cb` - but that does depend on what to actually store. 

But let me know if you have other suggestions for an implementation.