Added js module: new JavaScript API supporting V8 by snej · Pull Request #91 · couchbase/sg-bucket

snej · 2023-06-13T16:41:25Z

Part of couchbase/sync_gateway#5980 -- I just moved the js module here, and the v8go package dependency.

torcolvin

A bunch of pedantic nitpicks, but shouldn't

js_map_fn.go, js_runner.go, js_server.go

be obsolete with this commit + corresponding SG commit?

torcolvin · 2023-06-14T15:14:12Z

+	"fmt"
+	"strconv"
+
+	"github.com/pkg/errors"


Suggested change

"github.com/pkg/errors"

"errors"

Now that these are present in go 1.20

torcolvin · 2023-06-14T15:36:49Z

+		return nil, fmt.Errorf("the js.VM has been closed")
+	}
+	if vm.curRunner != nil {
+		panic("illegal access to v8VM: already has a v8Runner")


Suggested change

panic("illegal access to v8VM: already has a v8Runner")

panic("illegal access to ottoVM: already has a ottoRunner")

torcolvin · 2023-06-14T15:37:08Z

+		panic("illegal access to v8VM: already has a v8Runner")
+	}
+	if !vm.services.hasService(service) {
+		return nil, fmt.Errorf("unknown js.Service instance passed to VM")


Suggested change

return nil, fmt.Errorf("unknown js.Service instance passed to VM")

return nil, fmt.Errorf("unknown js.Service instance passed to VM %v", service)

torcolvin · 2023-06-14T15:37:45Z

+	"strings"
+	"time"
+
+	"github.com/pkg/errors"


Suggested change

"github.com/pkg/errors"

Since this is included go 1.20 now

torcolvin · 2023-06-14T15:38:35Z

+	// Create a V8 Context and run the setup script in it:
+	ctx := v8.NewContext(vm.iso, template.Global())
+	if _, err := vm.setupScript.Run(ctx); err != nil {
+		return nil, errors.Wrap(err, "Unexpected error in JavaScript initialization code")


do we need ctx.Close() here?

torcolvin · 2023-06-14T15:41:22Z

+	// if httpErr, ok := err.(*base.HTTPError); ok {
+	// 	errStr = fmt.Sprintf("[%d] %s", httpErr.Status, httpErr.Message)
+	// } else {


delete this code? or keep it?

torcolvin · 2023-06-14T15:41:38Z

+	"sync"
+	"time"
+
+	"github.com/pkg/errors"


Suggested change

"github.com/pkg/errors"

torcolvin · 2023-06-14T15:43:02Z

+func (vm *v8VM) getTemplate(service *Service) (V8Template, error) {
+	var tmpl V8Template
+	if !vm.services.hasService(service) {
+		return nil, fmt.Errorf("unknown js.Service instance passed to VM")


Suggested change

return nil, fmt.Errorf("unknown js.Service instance passed to VM")

return nil, fmt.Errorf("unknown js.Service instance passed to VM %v", service)

torcolvin · 2023-06-14T15:43:35Z

+			var err error
+			vm.setupScript, err = vm.iso.CompileUnboundScript(kSetupLoggingJS+kUnderscoreJS, "setupScript.js", v8.CompileOptions{})
+			if err != nil {
+				return nil, errors.Wrapf(err, "Couldn't compile setup script")


Suggested change

return nil, errors.Wrapf(err, "Couldn't compile setup script")

return nil, fmt.Errorf("Couldn't compile setup script: %w")

torcolvin · 2023-06-14T21:55:38Z

+
+func TestPoolsConcurrently(t *testing.T) {
+	maxProcs := runtime.GOMAXPROCS(0)
+	log.Printf("FYI, GOMAXPROCS = %d", maxProcs)


Suggested change

log.Printf("FYI, GOMAXPROCS = %d", maxProcs)

torcolvin

The second set of changes I'd like to make are:

is to make the code panic-less, even though usually this amounts to programmer error if there's a problem that would case a panic. The reason for this is that we could bring down a SG process with multiple databases, where only would fail in a single database, even if it doesn't start up, or fails to update sync function
pass contexts into the functions where possible, since we are using them more and more for logging. Instead of using context.Background() replace with context.TODO(). Some of these contexts are inside the js code only, but some of these functions I think call back into sync gateway where the context logging could be helpful to us.

- VM and VMPool now have an associated Context, which must be given when creating one. (VMs created by VMPool inherit its Context.) - VM and VMPool now have a Context() method. - Runner.Context() now returns the VM's Context by default, but calling Runner.SetContext() overrides it until the Runner is returned. - Removed Runner.ContextOrDefault(); use Context() instead. - The `ctx` parameter to Service.Run() may be nil, in which case the default Context (the VM's) is used. - Tests use a new testCtx() function that's similar to the one in SG.base. It creates a Context derived from context.TODO that cancels when the test exits. Some renaming to avoid confusion: - Renamed v8Runner.ctx to v8ctx. - Renamed baseRunner.goContext to ctx. - Renamed other variables of type *v8.Context to v8ctx.

snej · 2023-06-21T00:19:23Z

I've implemented change 2, adding Contexts, in commit c770934. See the commit log for details.

The addlicense check is failing; it needs to be disabled for js/underscore-umd-min.js and js/underscore-umd.js .

- panics in illegal situations replaced by logError() and returning, with an error if possible. - V8Runner.NewInt() and NewString() now return an error because the V8 call returns an error, although I believe it can only fail in the unlikely case that V8 runs out of memory.

snej · 2023-06-21T23:37:07Z

Change 1 (no panics) now implemented in 5ee3866. This required a few change in sync_gateway, which I've also pushed.

* add v8 tests * Ignore underscore files * use quotes

Added js module: new JavaScript API supporting V8

30840f2

snej mentioned this pull request Jun 8, 2023

Use V8 instead of Otto for JavaScript couchbase/sync_gateway#5980

Closed

6 tasks

torcolvin reviewed Jun 15, 2023

View reviewed changes

torcolvin assigned snej Jun 20, 2023

Addressing review comments

fa73ec2

snej force-pushed the feature/v8 branch from 5ed774f to fa73ec2 Compare June 20, 2023 21:00

snej added 2 commits June 20, 2023 14:43

Added license text

88ea081

snej force-pushed the feature/v8 branch from 094b02e to c770934 Compare June 21, 2023 21:05

v8go package moved from snej to couchbasedeps

3779b87

snej force-pushed the feature/v8 branch from c0732b0 to 3779b87 Compare July 11, 2023 21:29

torcolvin approved these changes Jul 11, 2023

View reviewed changes

Add v8 CI (#95)

7e6d70b

* add v8 tests * Ignore underscore files * use quotes

	panic("illegal access to v8VM: already has a v8Runner")
	panic("illegal access to ottoVM: already has a ottoRunner")

	return nil, fmt.Errorf("unknown js.Service instance passed to VM")
	return nil, fmt.Errorf("unknown js.Service instance passed to VM %v", service)

	return nil, errors.Wrapf(err, "Couldn't compile setup script")
	return nil, fmt.Errorf("Couldn't compile setup script: %w")

Conversation

snej commented Jun 13, 2023

Uh oh!

torcolvin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

torcolvin left a comment

Choose a reason for hiding this comment

Uh oh!

snej commented Jun 21, 2023

Uh oh!

snej commented Jun 21, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants