Added js module: new JavaScript API supporting V8 #91
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
6 tasks
torcolvin
reviewed
Jun 15, 2023
js/otto_runner.go
Outdated
| "fmt" | ||
| "strconv" | ||
|
|
||
| "github.com/pkg/errors" |
There was a problem hiding this comment.
Suggested change
| "github.com/pkg/errors" | |
| "errors" |
Now that these are present in go 1.20
js/otto_vm.go
Outdated
| return nil, fmt.Errorf("the js.VM has been closed") | ||
| } | ||
| if vm.curRunner != nil { | ||
| panic("illegal access to v8VM: already has a v8Runner") |
There was a problem hiding this comment.
Suggested change
| panic("illegal access to v8VM: already has a v8Runner") | |
| panic("illegal access to ottoVM: already has a ottoRunner") |
js/otto_vm.go
Outdated
| 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") |
There was a problem hiding this comment.
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) |
js/v8_runner.go
Outdated
| "strings" | ||
| "time" | ||
|
|
||
| "github.com/pkg/errors" |
There was a problem hiding this comment.
Suggested change
| "github.com/pkg/errors" |
Since this is included go 1.20 now
js/v8_runner.go
Outdated
| // 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") |
There was a problem hiding this comment.
do we need ctx.Close() here?
js/v8_utils.go
Outdated
Comment on lines
118
to
120
| // if httpErr, ok := err.(*base.HTTPError); ok { | ||
| // errStr = fmt.Sprintf("[%d] %s", httpErr.Status, httpErr.Message) | ||
| // } else { |
There was a problem hiding this comment.
delete this code? or keep it?
js/v8_vm.go
Outdated
| "sync" | ||
| "time" | ||
|
|
||
| "github.com/pkg/errors" |
There was a problem hiding this comment.
Suggested change
| "github.com/pkg/errors" |
js/v8_vm.go
Outdated
| 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") |
There was a problem hiding this comment.
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) |
js/v8_vm.go
Outdated
| 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") |
There was a problem hiding this comment.
Suggested change
| return nil, errors.Wrapf(err, "Couldn't compile setup script") | |
| return nil, fmt.Errorf("Couldn't compile setup script: %w") |
js/vmpool_test.go
Outdated
|
|
||
| func TestPoolsConcurrently(t *testing.T) { | ||
| maxProcs := runtime.GOMAXPROCS(0) | ||
| log.Printf("FYI, GOMAXPROCS = %d", maxProcs) |
There was a problem hiding this comment.
Suggested change
| log.Printf("FYI, GOMAXPROCS = %d", maxProcs) |
torcolvin
reviewed
Jun 15, 2023
There was a problem hiding this comment.
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 withcontext.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.
|
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.
|
Change 1 (no panics) now implemented in 5ee3866. This required a few change in sync_gateway, which I've also pushed. |
torcolvin
approved these changes
Jul 11, 2023
* add v8 tests * Ignore underscore files * use quotes
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Part of couchbase/sync_gateway#5980 -- I just moved the
jsmodule here, and thev8gopackage dependency.