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 resolver and promise result #76

Merged
merged 7 commits into from
Mar 7, 2021
Merged

Promise resolver and promise result #76

merged 7 commits into from
Mar 7, 2021

Conversation

rogchap
Copy link
Owner

@rogchap rogchap commented Feb 11, 2021

Enables the (basic) Promise API. eg:

iso, _ := v8go.NewIsolate()
global, _ := v8go.NewObjectTemplate(iso)

fetchfn, _ := v8go.NewFunctionTemplate(iso, func(info *v8go.FunctionCallbackInfo) *v8go.Value {
	args := info.Args()
	url := args[0].String()
	resolver, _ := v8go.NewPromiseResolver(info.Context())

	go func() {
		res, _ := http.Get(url)
		body, _ := ioutil.ReadAll(res.Body)
		val, _ := v8go.NewValue(iso, string(body))
		resolver.Resolve(val)
	}()
	return resolver.GetPromise().Value
})
global.Set("fetch", fetchfn, v8go.ReadOnly)

ctx, _ := v8go.NewContext(iso, global)
val, _ := ctx.RunScript("fetch('https://rogchap.com/v8go')", "")
prom, _ := val.AsPromise()

// wait for the promise to resolve
for prom.State() == v8go.Pending {
	continue
}
fmt.Printf("%s\n", strings.Split(prom.Result().String(), "\n")[0])
// Output:
// <!DOCTYPE html>

resolves #69

@codecov
Copy link

codecov bot commented Feb 11, 2021

Codecov Report

Merging #76 (d233d29) into master (9848a00) will increase coverage by 0.57%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #76      +/-   ##
==========================================
+ Coverage   94.07%   94.64%   +0.57%     
==========================================
  Files          10       11       +1     
  Lines         388      411      +23     
==========================================
+ Hits          365      389      +24     
+ Misses         16       15       -1     
  Partials        7        7              
Impacted Files Coverage Δ
promise.go 100.00% <100.00%> (ø)
value.go 95.09% <100.00%> (+0.09%) ⬆️
function_template.go 100.00% <0.00%> (+4.34%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9848a00...d233d29. Read the comment docs.

@kuoruan
Copy link
Contributor

kuoruan commented Feb 22, 2021

Dose exception.go required when reject a promise?
Can I just reject a string?

e, _ := v8go.NewValue(iso, "any errors")
resolver.Reject(e)

@rogchap
Copy link
Owner Author

rogchap commented Feb 22, 2021

You should be able to reject with any "Value" which includes a string, but it's most common to use an Error/Exception

@rogchap
Copy link
Owner Author

rogchap commented Feb 23, 2021

Just a quick update:

Sorry for taking so long in getting this PR finished; it's almost done.

Annoyingly, I'm in the process of moving house, so packing boxes has been taking most of my spare time 😞

Should be fully moved in by this time next week, and should be able to continue my work on v8go.

@jensneuse
Copy link

I'm not experienced with v8 but like to ask a question.
Looking at this code:

for prom.State() == v8go.Pending {
	continue
}

It seems, this is constantly calling the cgo bridge, doesn't it?
Will this affect runtime performance?
Would it make sense to blocking read on a channel until the v8 calls a resolved/rejected callback?

@rogchap
Copy link
Owner Author

rogchap commented Mar 2, 2021

@jensneuse Thanks for the suggestion. Hopefully this helps answer your questions:

It seems, this is constantly calling the cgo bridge, doesn't it?

It does 😓

Will this affect runtime performance?

Not really. The main issues around performance arise from transferring large amounts of data between C and Go and having to copy that data into memory on the Go side to make sure it does not get freed on the C side before you use it on the Go side. This func is returning an enum aka integer by value so you shouldn't notice any overhead in doing this in Go over doing the same thing in C++.

Would it make sense to blocking read on a channel until the v8 calls a resolved/rejected callback?

It's a nice idea, but one of the goals of the project is stay as close to the V8 API as possible; it also has a lot larger overhead as callbacks are not an easy thing to manage between Go and C (Have a look at the FunctionTemplate code to get an idea of the complexity)

@rogchap
Copy link
Owner Author

rogchap commented Mar 2, 2021

I'm having a hard time with getting Error Exceptions working. This simple line is causing a segfault:

Exception::Error(String::Empty(iso));

I've verified that the Isolate* is valid as I can create other values, just not Exception value. 🤷‍♂️

It's not critical for this PR to have the Exception/Error objects, but I thought it would be nice for the Promise rejections.
As noted above, you can use any Value as the value to the Reject function.

I'm tempted to exclude the exception.go stuff (just make it private for now) and revisit in another PR.

@rogchap rogchap requested review from tmc and zwang March 2, 2021 11:32
@rogchap rogchap marked this pull request as ready for review March 2, 2021 11:32
@tmc
Copy link
Collaborator

tmc commented Mar 3, 2021

I'm tempted to exclude the exception.go stuff (just make it private for now) and revisit in another PR.

It seems like that can come in a follow-on commit.

Copy link
Collaborator

@tmc tmc left a comment

Choose a reason for hiding this comment

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

LGTM, some small comments.

// The associated Promise will be in a Pending state.
func NewPromiseResolver(ctx *Context) (*PromiseResolver, error) {
if ctx == nil {
return nil, errors.New("v8go: Context is required")
Copy link
Collaborator

Choose a reason for hiding this comment

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

nite: lowercase?

@kuoruan
Copy link
Contributor

kuoruan commented Mar 3, 2021

Hi @rogchap
V8 v8.9 now in stable channel.
https://omahaproxy.appspot.com/ shows the version is 8.9.255.20

It supports top level await. https://v8.dev/blog/v8-release-89#top-level-await

Could you please update the V8 binary file version?

@rogchap
Copy link
Owner Author

rogchap commented Mar 3, 2021

@kuoruan Top level await only works at the top level of a module not at the top top level; which is a bit miss leading.

It's still behind a feature flag but I'll look to see if we can add it and upgrade v8 to the latest stable version.

I'll do this in a seperate PR

}

// Reject
func (r *PromiseResolver) Reject(err *Value) bool {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not change this to Valuer?
So that we can reject an object here

@lebronyang1
Copy link

for prom.State() == v8go.Pending {
continue
}

@rogchap Hi, I don't always wait for promise results, Are there more effective ways to resolve this problem?
Wether I can allows the isolate execute other context, when wait promise results? In other words, How do I use the isolate in multiple threads? Especially promise, I think it's so slowly.

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.

[feature] How to get the Promise result value?
6 participants