MaybeLocal and Maybe versions of GetInternalField and GetAlignedPointerFromInternalField

[James Snell]

Currently existing value checks from GetInternalField and GetAlignedPointerFromInternalField can be a bit awkward. The former returns a Local<Value>, while the latter returns a void* and asserts if the value is not an aligned pointer.

I would like to explore adding a pair of alternatives that would return a MaybeLocal<Value> and Maybe<T>, respectively... for instance:

```

MaybeLocal<Value> ret = obj->GetInternalFieldMaybe(0);

if (!ret.IsEmpty()) {

  Local<Value> val = ret.ToLocalChecked();

  // do something with val

}

```

and

```

Foo* foo = nullptr;

Maybe<Foo> ret = obj->GetAlignedPointerFromInternalFieldMaybe(0);

if (ret.To(foo))

  // do something with foo

```

A key difference in behavior with the GetAlignedPointerFromInternalFieldMaybe variant is that instead of asserting when not an aligned pointer, it would return a Maybe with IsNothing == true. One use case for this we have in Node.js is that we currently use an internal field in the Promise object to store a reference to a tracking object (part of our async context tracking mechanism). That is stored using SetInternalField, but we have a check just prior to make sure there's not an aligned pointer set in that field already, which will assert if SetInternalField has already been called.

Before going off and implementing, I wanted to (a) double check to make sure this even made sense and (b) ask for alternative naming suggestions because `GetInternalFieldMaybe()` really isn't that great unless I was Carly Rae Jepsen.

- James

[Yuki Shiino]

Blink-V8 bindings team expects (and is relying on) a convention that, if a Maybe/MaybeLocal is empty, then an exception is being thrown.  This is very important when implementing V8 callback functions (IDL attribute callback, IDL operation callback, etc.).

v8::MaybeLocal<v8::Value> maybe_value = Func();

v8::Local<v8::Value> value;

if (!maybe_value.ToLocal(&value)) {

  return;  // Just returns because an exception must be thrown already.

}

// Use |value|.

I personally would like V8 APIs to keep this convention.  In case of GetAlignedPointerFromInternalField, I'm afraid that there wouldn't be a proper exception to be thrown.  Maybe, base::Optional<T> would fit better?

Cheers,

Yuki Shiino

2020年2月27日(木) 8:20 James Snell <jas...@gmail.com>:

--
--
v8-dev mailing list
v8-...@googlegroups.com
http://groups.google.com/group/v8-dev
---
You received this message because you are subscribed to the Google Groups "v8-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to v8-dev+un...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/v8-dev/8d6fa27c-87cc-422d-8fb1-cf8e34fefa95%40googlegroups.com.

[James Snell]

Hmm... as far as I know base::Optional is not currently used anywhere else in the public v8 API correct?

An alternative here that avoids the exception issue could be:

1. For GetInternalFieldMaybe(), if the value is not set, it returns Undefined rather than empty. Not ideal but certainly something we can work with.

2. For GetAlignedPointerFromInternalField() we could use a pattern more like To() ... that is:

    bool GetAlignedPointerFromInternalField(int field, void* ptr)

    Such that if the internal field is an aligned pointer, ptr is set and true is returned, otherwise ptr is untouched and false is returned. With no assert. It's less ideal but also something we can work with.

- James

To unsubscribe from this group and stop receiving emails from it, send an email to v8-...@googlegroups.com.

[Yuki Shiino]

I don't have a strong opinion except for the convention of throwing an exception.

2020年2月27日(木) 23:57 James Snell <jas...@gmail.com>:

To unsubscribe from this group and stop receiving emails from it, send an email to v8-dev+un...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/v8-dev/e811b00c-d173-4c7f-8a7a-c52d0676cd50%40googlegroups.com.

[James M Snell]

Ok! Thank you for the feedback! Very much appreciated

You received this message because you are subscribed to a topic in the Google Groups "v8-dev" group.
To unsubscribe from this topic, visit https://groups.google.com/d/topic/v8-dev/fk2WLvhONuk/unsubscribe.
To unsubscribe from this group and all its topics, send an email to v8-dev+un...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/v8-dev/CAN0uC_Sx5i2sZM4eB%3DRJM%2BdDeL2e96ANor3KbaLgjOS8%2B6UwDQ%40mail.gmail.com.

[Jakob Kummerow]

I am not an API owner, so I can only offer some generic background here: generally, we use Maybe and MaybeLocal for things that can fail when a JavaScript exception is thrown, which in particular means that these failures can happen even when the C++ code is perfectly bug-free. (This ties in to the invariant that Yuki pointed out: "maybe.IsNothing()" should be true iff "i::isolate->has_pending_exception()".) CHECKs and DCHECKs, on the other hand, are used for things that are not supposed to ever happen if the C++ code is correct.

GetAlignedPointerFromInternalField has traditionally been seen as being in the latter bucket: if an embedder calls that, and the field does not contain an aligned pointer, then that embedder's code is incorrect and should crash. It sounds to me like what you're doing is you're using the same internal field to sometimes store an aligned pointer and sometimes store a heap object, is that correct? The cleanest solution, given the existing architecture, would probably be to use separate fields.

(As for your naming question: a common pattern is to use names like TryGetFoo; but considering the above I'm not sure a Maybe-typed interface is the way to go in this case.)

To view this discussion on the web visit https://groups.google.com/d/msgid/v8-dev/CABP7RbdHYqbOG7aGQ1v6qvo8V2KBqv8UPJaXHoTrLhisd9f-Gw%40mail.gmail.com.

[James M Snell]

On Fri, Feb 28, 2020 at 2:41 AM Jakob Kummerow <jkum...@chromium.org> wrote:

I am not an API owner, so I can only offer some generic background here: generally, we use Maybe and MaybeLocal for things that can fail when a JavaScript exception is thrown, which in particular means that these failures can happen even when the C++ code is perfectly bug-free. (This ties in to the invariant that Yuki pointed out: "maybe.IsNothing()" should be true iff "i::isolate->has_pending_exception()".) CHECKs and DCHECKs, on the other hand, are used for things that are not supposed to ever happen if the C++ code is correct.

GetAlignedPointerFromInternalField has traditionally been seen as being in the latter bucket: if an embedder calls that, and the field does not contain an aligned pointer, then that embedder's code is incorrect and should crash. It sounds to me like what you're doing is you're using the same internal field to sometimes store an aligned pointer and sometimes store a heap object, is that correct? The cleanest solution, given the existing architecture, would probably be to use separate fields.

The existing code only ever stores a Local<Value> using SetInternalField but given that the Promise we're storing into can come from anywhere the idea is to be defensive and check first that the internal field is not already used.

Thinking about it more, I think we can get by with a simple existence check, something like IsAlignedPointerInInternalField(index) that returns a bool or Maybe<bool> would work just fine.

- James

 

To view this discussion on the web visit https://groups.google.com/d/msgid/v8-dev/CAKSzg3RDPzKn0da%3D623VKg2CaPBC1k0xtETVbAmJngTHifX6Zw%40mail.gmail.com.