Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

promise: run callbacks on resolution #118

Merged
merged 6 commits into from
May 6, 2021
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions context.go
Original file line number Diff line number Diff line change
Expand Up @@ -117,6 +117,14 @@ func (c *Context) Global() *Object {
return &Object{v}
}

// PerformMicrotaskCheckpoint runs the default MicrotaskQueue until empty.
// This is used to make progress on Promises.
func (c *Context) PerformMicrotaskCheckpoint() {
c.register()
defer c.deregister()
C.IsolatePerformMicrotaskCheckpoint(c.iso.ptr)
}

// Close will dispose the context and free the memory.
// Access to any values assosiated with the context after calling Close may panic.
func (c *Context) Close() {
Expand Down
38 changes: 38 additions & 0 deletions promise.go
Original file line number Diff line number Diff line change
Expand Up @@ -85,3 +85,41 @@ func (p *Promise) Result() *Value {
val := &Value{ptr, p.ctx}
return val
}

// Then accepts 1 or 2 callbacks.
// The first is invoked when the promise has been fulfilled.
// The second is invoked when the promise has been rejected.
// The returned Promise resolves after the callback finishes execution.
//
// V8 only invokes the callback when processing "microtasks".
// The default MicrotaskPolicy processes them when the call depth decreases to 0.
// Call (*Context).PerformMicrotaskCheckpoint to trigger it manually.
func (p *Promise) Then(cbs ...FunctionCallback) *Promise {
p.ctx.register()
defer p.ctx.deregister()

var ptr C.ValuePtr
switch len(cbs) {
case 1:
cbID := p.ctx.iso.registerCallback(cbs[0])
ptr = C.PromiseThen(p.ptr, C.int(cbID))
case 2:
cbID1 := p.ctx.iso.registerCallback(cbs[0])
cbID2 := p.ctx.iso.registerCallback(cbs[1])
ptr = C.PromiseThen2(p.ptr, C.int(cbID1), C.int(cbID2))

default:
panic("1 or 2 callbacks required")
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would like to avoid panic within this library and also just ignore Callbacks if the length in > 2. Can we just return p and make it a noop?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK. I think we really don't see eye to eye on this design question, since I feel adverse to this. IMO this goes against the typical design ethos & common practice.. that is, ignoring unexpected input and carrying on. But it's your library, and in this case the chance of mistakes by the caller seems low. Updated

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a fair comment and I agree. Would it be better to return an error? 🤔

The downside is you loose the ability to do method chaining (which is not generally idiomatic Go)

I'm just trying to avoid using panic

I'm open to other's input? @tmc @cleiner @zwang

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I prefer not panic solution as this is a library to be used by other app. Good not to panic.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be better to panic, hence the code :)

I would refer you again to examples in the standard library:
https://pkg.go.dev/strings#Repeat

There are no fewer than 3 functions in the strings package that are documented to panic when given an invalid input. It would be frustrating for callers if they had to handle an error return unnecessarily. It's ok for the standard library to panic but not for v8go? The argument that panics are not allowed in public interfaces at all must not be telling the whole story.

Searching for more information on the topic, I agree with the point of view presented here by Axel:
https://groups.google.com/g/golang-nuts/c/5S324hgLdfI/m/XpWmVQGJCAAJ

Personally I've been using Go since 1.0 (... 10 years now?) and I've seen a lot of libraries. I do not believe the nil checks and error returns used in v8go are common practice at all. Can you point to popular packages that do this?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok; I think we are all in agreement now. Sorry for the back-and-forth @robfig can you revert your last commit? ie. place back the panic.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, happy to do it. Updated.

Last note - a lot of the errors returned in v8go are as a result of nil checks. In the standard library, it doesn't even perform nil checks, relying on the code to panic on nil as a side effect.

For example, it looks to me like io.WriteString(w Writer, s string) will panic if either of the arguments are nil.
https://golang.org/src/io/io.go?s=13817:13895#L307

I know there is a practice (which I've seen in Java in particular) to aggressively check arguments for nil on entry to every function. I haven't really seen that done in Go, and I can't say that I've missed it.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So it seem the conclusion is: panic is OK when arguments to the API are invalid, but for results of running JavaScript, there will still be err returned? So if JavaScript code throws a top-level exception, there will still be err and not panic?

Copy link
Owner

@rogchap rogchap May 6, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea, that seems like the right approach; JS exceptions would still return error

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK. So will you then change the API now? Like template.Set currently returns err. It seems it should not anymore and just panic?

}
return &Promise{&Object{&Value{ptr, p.ctx}}}
}

// Catch invokes the given function if the promise is rejected.
// See Then for other details.
func (p *Promise) Catch(cb FunctionCallback) *Promise {
p.ctx.register()
defer p.ctx.deregister()
cbID := p.ctx.iso.registerCallback(cb)
ptr := C.PromiseCatch(p.ptr, C.int(cbID))
return &Promise{&Object{&Value{ptr, p.ctx}}}
}
61 changes: 60 additions & 1 deletion promise_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ import (
"rogchap.com/v8go"
)

func TestPromise(t *testing.T) {
func TestPromiseFulfilled(t *testing.T) {
t.Parallel()

iso, _ := v8go.NewIsolate()
Expand All @@ -25,6 +25,19 @@ func TestPromise(t *testing.T) {
t.Errorf("unexpected state for Promise, want Pending (0) got: %v", s)
}

var thenInfo *v8go.FunctionCallbackInfo
prom1thenVal := prom1.Then(func(info *v8go.FunctionCallbackInfo) *v8go.Value {
thenInfo = info
return nil
})
prom1then, _ := prom1thenVal.AsPromise()
if prom1then.State() != v8go.Pending {
t.Errorf("unexpected state for dependent Promise, want Pending got: %v", prom1then.State())
}
if thenInfo != nil {
t.Error("unexpected call of Then prior to resolving the promise")
}

val1, _ := v8go.NewValue(iso, "foo")
res1.Resolve(val1)

Expand All @@ -36,6 +49,20 @@ func TestPromise(t *testing.T) {
t.Errorf("expected the Promise result to match the resolve value, but got: %s", result)
}

if thenInfo == nil {
t.Errorf("expected Then to be called, was not")
}
if len(thenInfo.Args()) != 1 || thenInfo.Args()[0].String() != "foo" {
t.Errorf("expected promise to be called with [foo] args, was: %+v", thenInfo.Args())
}
}

func TestPromiseRejected(t *testing.T) {
t.Parallel()

iso, _ := v8go.NewIsolate()
ctx, _ := v8go.NewContext(iso)

res2, _ := v8go.NewPromiseResolver(ctx)
val2, _ := v8go.NewValue(iso, "Bad Foo")
res2.Reject(val2)
Expand All @@ -44,4 +71,36 @@ func TestPromise(t *testing.T) {
if s := prom2.State(); s != v8go.Rejected {
t.Fatalf("unexpected state for Promise, want Rejected (2) got: %v", s)
}

var thenInfo *v8go.FunctionCallbackInfo
var then2Fulfilled, then2Rejected bool
prom2.
Catch(func(info *v8go.FunctionCallbackInfo) *v8go.Value {
thenInfo = info
return nil
}).
Then(
func(_ *v8go.FunctionCallbackInfo) *v8go.Value {
then2Fulfilled = true
return nil
},
func(_ *v8go.FunctionCallbackInfo) *v8go.Value {
then2Rejected = true
return nil
},
)
ctx.PerformMicrotaskCheckpoint()
if thenInfo == nil {
t.Fatalf("expected Then to be called on already-resolved promise, but was not")
}
if len(thenInfo.Args()) != 1 || thenInfo.Args()[0].String() != val2.String() {
t.Fatalf("expected [%v], was: %+v", val2, thenInfo.Args())
}

if then2Fulfilled {
t.Fatalf("unexpectedly called onFulfilled")
}
if !then2Rejected {
t.Fatalf("expected call to onRejected, got none")
}
}
53 changes: 53 additions & 0 deletions v8go.cc
Original file line number Diff line number Diff line change
Expand Up @@ -160,6 +160,11 @@ IsolatePtr NewIsolate() {
return static_cast<IsolatePtr>(iso);
}

void IsolatePerformMicrotaskCheckpoint(IsolatePtr ptr) {
ISOLATE_SCOPE(ptr)
iso->PerformMicrotaskCheckpoint();
}

void IsolateDispose(IsolatePtr ptr) {
if (ptr == nullptr) {
return;
Expand Down Expand Up @@ -1077,6 +1082,54 @@ int PromiseState(ValuePtr ptr) {
return promise->State();
}

ValuePtr PromiseThen(ValuePtr ptr, int callback_ref) {
LOCAL_VALUE(ptr)
Local<Promise> promise = value.As<Promise>();
Local<Integer> cbData = Integer::New(iso, callback_ref);
Local<Function> func = Function::New(local_ctx, FunctionTemplateCallback, cbData)
.ToLocalChecked();
Local<Promise> result = promise->Then(local_ctx, func).ToLocalChecked();
m_value* promise_val = new m_value;
promise_val->iso = iso;
promise_val->ctx = ctx;
promise_val->ptr =
Persistent<Value, CopyablePersistentTraits<Value>>(iso, promise);
return tracked_value(ctx, promise_val);
}

ValuePtr PromiseThen2(ValuePtr ptr, int on_fulfilled_ref, int on_rejected_ref) {
LOCAL_VALUE(ptr)
Local<Promise> promise = value.As<Promise>();
Local<Integer> onFulfilledData = Integer::New(iso, on_fulfilled_ref);
Local<Function> onFulfilledFunc = Function::New(local_ctx, FunctionTemplateCallback, onFulfilledData)
.ToLocalChecked();
Local<Integer> onRejectedData = Integer::New(iso, on_rejected_ref);
Local<Function> onRejectedFunc = Function::New(local_ctx, FunctionTemplateCallback, onRejectedData)
.ToLocalChecked();
Local<Promise> result = promise->Then(local_ctx, onFulfilledFunc, onRejectedFunc).ToLocalChecked();
m_value* promise_val = new m_value;
promise_val->iso = iso;
promise_val->ctx = ctx;
promise_val->ptr =
Persistent<Value, CopyablePersistentTraits<Value>>(iso, promise);
return tracked_value(ctx, promise_val);
}

ValuePtr PromiseCatch(ValuePtr ptr, int callback_ref) {
LOCAL_VALUE(ptr)
Local<Promise> promise = value.As<Promise>();
Local<Integer> cbData = Integer::New(iso, callback_ref);
Local<Function> func = Function::New(local_ctx, FunctionTemplateCallback, cbData)
.ToLocalChecked();
Local<Promise> result = promise->Catch(local_ctx, func).ToLocalChecked();
m_value* promise_val = new m_value;
promise_val->iso = iso;
promise_val->ctx = ctx;
promise_val->ptr =
Persistent<Value, CopyablePersistentTraits<Value>>(iso, promise);
return tracked_value(ctx, promise_val);
}

ValuePtr PromiseResult(ValuePtr ptr) {
LOCAL_VALUE(ptr)
Local<Promise> promise = value.As<Promise>();
Expand Down
4 changes: 4 additions & 0 deletions v8go.h
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@ typedef struct {

extern void Init();
extern IsolatePtr NewIsolate();
extern void IsolatePerformMicrotaskCheckpoint(IsolatePtr ptr);
extern void IsolateDispose(IsolatePtr ptr);
extern void IsolateTerminateExecution(IsolatePtr ptr);
extern IsolateHStatistics IsolationGetHeapStatistics(IsolatePtr ptr);
Expand Down Expand Up @@ -171,6 +172,9 @@ extern ValuePtr PromiseResolverGetPromise(ValuePtr ptr);
int PromiseResolverResolve(ValuePtr ptr, ValuePtr val_ptr);
int PromiseResolverReject(ValuePtr ptr, ValuePtr val_ptr);
int PromiseState(ValuePtr ptr);
ValuePtr PromiseThen(ValuePtr ptr, int callback_ref);
ValuePtr PromiseThen2(ValuePtr ptr, int on_fulfilled_ref, int on_rejected_ref);
ValuePtr PromiseCatch(ValuePtr ptr, int callback_ref);
extern ValuePtr PromiseResult(ValuePtr ptr);

extern RtnValue FunctionCall(ValuePtr ptr, int argc, ValuePtr argv[]);
Expand Down