Use V8 instead of Otto for JavaScript #5980

snej · 2022-12-16T22:54:39Z

CBG-2949
Replaces Otto with V8 for all uses of JavaScript.

The new js package is a general purpose thread-safe library for creating "services" that call a JavaScript function in an isolated runtime context.

Pre-review checklist

Removed debug logging (fmt.Print, log.Print, ...)
Logging sensitive data? Make sure it's tagged (e.g. base.UD(docID), base.MD(dbName))
Updated relevant information in the API specifications (such as endpoint descriptions, schemas, ...) in docs/api

Dependencies (if applicable)

Link upstream PRs -- Added js module: new JavaScript API supporting V8 sg-bucket#91
Update Go module dependencies when merged

Integration Tests

GSI=true,xattrs=true https://jenkins.sgwdev.com/job/SyncGateway-Integration/000/

Functions: Moved limits into config itself - Functions config is now a struct not a map - The map is now its Definitions field - Added limits to the struct - Added limits to GraphQLConfig - REST and BLIP clients check the max_request_size limit

per latest spec

- delete(docID) deletes the doc with that ID. - delete(body) uses body._id (required) and body._rev (optional). If _rev is given, uses MVCC.

This isn't 100% reliable because it just uses a simple regex. A mutating query could be put after a SELECT and a semicolon. I think there might also be clauses that can validly go before SELECT (?)

(parent, args, context, info) ... not (context, args, parent, info)

Now uses ${...} and supports multiple path components and array indexing.

Field resolvers of the Query and Mutation types should treat a missing "allow" object as meaning admin-only, the way functions do.

I added a js.VMPool to Database. Since UserFunctions and GraphQL require the VMPool to initialize themselves, those objects can't be part of the DatabaseContextOptions, which is created earlier. So I defined an interface for the functions/GraphQL configuration (which is defined up in the functions package) and put that on the Options instead.

sync.Pool drops objects fairly often, and the VM objects and their services are pretty expensive to recreate; this was bad for performance.

* CBG-3024 Make sure import feed uses checkpoints

* CBG-3001 Avoid bucket retrieval error during OnFeedClose OnFeedError was checking for bucket existence to determine whether to call NotifyMgrOnClose(). This handling isn't necessary for SG, as we want database close to handle shutdown of the import feed (via importListener.Stop()) in the case of a deleted bucket.

* CBG-3028: fixes for failing CE tests * remove print line * updates off comment

…6280)

…6258) * CBG-2793: attachment compaction code erroneously sets failOnRollback * spelling errors and test logging removal + lint error * updates for comments and try fix race condition * linter error fix * changes for jenkins * updates to add handling for cleanup phase * remove logging line * fix race in Jenkins * updates to fix collections issue * updates based off comment * updates * updates to address complexity comments

torcolvin

I didn't review all of this code by any means.

Things I didn't review due to time constraints:

any JS/typescript code
understanding the GetDeepMutableBody and the ShallowCopy in the crud code.

channels/channelmapper.go

torcolvin · 2023-06-06T00:59:09Z

db/functions/engine/README.md

+On a clean build, or after any changes in the `engine` source tree:
+
+1. `cd db/functions/engine`
+2. `npm run build`


Is it worth writing some CI to make sure that we run the tests here and that if running npm run build changes, it fails if we don't update the file?

That could be good, but I myself am not good at CI stuff so I don't trust myself to write it.

channels/channelmapper.go

torcolvin · 2023-06-06T01:09:59Z

db/database_test.go

-func init() {
-	underscore.Disable() // It really slows down unit tests (by making otto.New take a lot longer)
-}


we are still running some of these tests with otto - that said, I'd be fine with putting this into a separate PR and moving this to main_test.go for relevant tests - and we can remove when we remove otto.

I took that line out because it doesn't make a significant difference anymore, i.e. the tests don't really take longer with it removed. I remember adding that back in 2013 or so, and either MacBooks have gotten a lot faster or the Go runtime has (probably more of the latter.)

torcolvin · 2023-06-06T01:13:33Z

db/crud.go

 		base.DebugfCtx(ctx, base.KeyCRUD, "updateDoc(%q): Rev %q causes %q to become current again",
 			base.UD(doc.ID), newRevID, doc.CurrentRev)
-		channelSet, access, roles, syncExpiry, oldBodyJSON, err = db.getChannelsAndAccess(ctx, doc, curBody, metaMap, doc.CurrentRev)
+		channelSet, access, roles, syncExpiry, oldBodyJSON, err = db.getChannelsAndAccess(ctx, doc, curBodyBytes, metaMap, doc.CurrentRev, false /*TODO: Is false correct??*/)


address this TODO?

Ah, right. false is correct, so I'll remove the comment.

torcolvin · 2023-06-06T01:37:05Z

db/import.go

+	return func(ctx context.Context, doc Body) (bool, error) {
+		if timeout > 0 {
+			var cancelFn context.CancelFunc
+			ctx, cancelFn = context.WithTimeout(ctx, timeout)


otto doesn't use context cancellation, is this being obeyed in otto?

Yup, see the other comment above (it's handled in js.OttoRunner.)

torcolvin · 2023-06-06T01:37:36Z

db/import.go

+		case bool:
+			return result, nil
+		case string:
+			return strconv.ParseBool(result)


What's the justification for dropping this error handling?

I don't think I dropped anything; the equivalent code in the master branch does the same thing, only with some extra no-op lines that unpack and repack the result.

torcolvin · 2023-06-06T01:38:06Z

db/import.go

+		case string:
+			return strconv.ParseBool(result)
+		}
+		return false, fmt.Errorf("import filter function returned non-boolean type %T", result)


Suggested change

return false, fmt.Errorf("import filter function returned non-boolean type %T", result)

return false, fmt.Errorf("import filter function returned non-boolean type %T: %+v", result, result)

torcolvin · 2023-06-06T01:40:33Z

rest/config.go

 					multiError = multiError.Append(fmt.Errorf("collection %q sync function error: %w", collectionName, err))
 				} else if isEmpty {
 					collectionConfig.SyncFn = nil
 				}

-				if isEmpty, err := validateJavascriptFunction(collectionConfig.ImportFilter); err != nil {
+				if isEmpty, err := validateJavascriptFunction(jsvm, collectionConfig.ImportFilter, 1, 99); err != nil {


you allow 999 args above, and this makes me think you want to put some constants for minJSArgs, maxImportFilterArguments

It looks like an import filter only takes one arg, so I'll change the 999 and the 99 to 1.

rest/persistent_config_test.go

torcolvin · 2023-06-06T19:18:37Z

db/crud.go

-	if err != nil {
-		return "", nil, err
-	}
+	localDocBody := localDoc.Body().ShallowCopy()


This needs to be GetDeepMutableBody() for otto and probably Body().ShallowCopy() for V8 - this is enough that I'd be tempted to write a shim to get the right part of the body?

I think not interacting with otto as go objects directly has the potential to slow down otto, which I don't think we want to do.

Otto exposes Go objects directly to JS, meaning that the resolver fn is capable of modifying the document maps. A conflict resolver isn't allowed to do that, so make deep copies of both docs before calling it.

snej · 2023-06-08T16:51:50Z

Hm, got a single test failure: in "test v8 (ubuntu) (pull_request)" the test TestReconnectReplicator failed:

2023-06-07T23:42:31.2727087Z     utilities_testing_resttester.go:199: 
2023-06-07T23:42:31.2727596Z         	Error Trace:	/home/runner/work/sync_gateway/sync_gateway/rest/utilities_testing_resttester.go:199
2023-06-07T23:42:31.2728496Z         	            				/home/runner/work/sync_gateway/sync_gateway/rest/utilities_testing_resttester.go:203
2023-06-07T23:42:31.2729429Z         	            				/home/runner/work/sync_gateway/sync_gateway/rest/replicatortest/replicator_test.go:2451
2023-06-07T23:42:31.2729672Z         	Error:      	Received unexpected error:
2023-06-07T23:42:31.2730325Z         	            	RetryLoop for Wait for condition options giving up after 201 attempts
2023-06-07T23:42:31.2730658Z         	Test:       	TestReconnectReplicator/max_backoff_not_specified
2023-06-07T23:42:31.2731010Z         	Messages:   	Expected status: reconnecting, actual status: running

torcolvin · 2023-06-08T16:54:52Z

Hm, got a single test failure: in "test v8 (ubuntu) (pull_request)" the test TestReconnectReplicator failed:

2023-06-07T23:42:31.2727087Z     utilities_testing_resttester.go:199: 
2023-06-07T23:42:31.2727596Z         	Error Trace:	/home/runner/work/sync_gateway/sync_gateway/rest/utilities_testing_resttester.go:199
2023-06-07T23:42:31.2728496Z         	            				/home/runner/work/sync_gateway/sync_gateway/rest/utilities_testing_resttester.go:203
2023-06-07T23:42:31.2729429Z         	            				/home/runner/work/sync_gateway/sync_gateway/rest/replicatortest/replicator_test.go:2451
2023-06-07T23:42:31.2729672Z         	Error:      	Received unexpected error:
2023-06-07T23:42:31.2730325Z         	            	RetryLoop for Wait for condition options giving up after 201 attempts
2023-06-07T23:42:31.2730658Z         	Test:       	TestReconnectReplicator/max_backoff_not_specified
2023-06-07T23:42:31.2731010Z         	Messages:   	Expected status: reconnecting, actual status: running

This is in a new test, @gregns1 can you have a look?

gregns1 · 2023-06-08T16:59:34Z

Hm, got a single test failure: in "test v8 (ubuntu) (pull_request)" the test TestReconnectReplicator failed:
2023-06-07T23:42:31.2727087Z utilities_testing_resttester.go:199:

2023-06-07T23:42:31.2727596Z Error Trace: /home/runner/work/sync_gateway/sync_gateway/rest/utilities_testing_resttester.go:199

2023-06-07T23:42:31.2728496Z /home/runner/work/sync_gateway/sync_gateway/rest/utilities_testing_resttester.go:203

2023-06-07T23:42:31.2729429Z /home/runner/work/sync_gateway/sync_gateway/rest/replicatortest/replicator_test.go:2451

2023-06-07T23:42:31.2729672Z Error: Received unexpected error:

2023-06-07T23:42:31.2730325Z RetryLoop for Wait for condition options giving up after 201 attempts

2023-06-07T23:42:31.2730658Z Test: TestReconnectReplicator/max_backoff_not_specified

2023-06-07T23:42:31.2731010Z Messages: Expected status: reconnecting, actual status: running
This is in a new test, @gregns1 can you have a look?

This looks like a race in the test. I'll have a look tomorrow to get a fix for it, you can ignore in the context of this PR.

snej · 2023-06-08T17:03:39Z

I can't even find that test! I've got feature/v8+subgraph checked out, it's up-to-date with Github, but the string TestReconnectReplicator doesn't exist in the source tree and definitely not at rest/replicatortest/replicator_test.go:2451. Am I losing my mind?

snej · 2023-06-08T17:07:22Z

The test log says HEAD is now at e326d6b Merge 0891f2eda0e468ff4354c2e23090e4a7674f2d17 into 3e36d801b8c6a659622d3d148dfcedd772c0b5ca -- that means it's merged master into the PR branch. I had no idea CI was testing these hypothetical merges ... I'm used to it testing the exact branch of the PR. Huh.

- Updated sg-bucket to latest (v8 branch) in go.mod. - Calls to V8BasicTemplate's and V8Runner's NewString() methods now return an error that needs to be checked. - Took out the 'mustSucceed()' utility since it can call panic; instead the error is now handled at the call site.

snej added 30 commits October 18, 2022 11:22

(fix REST tests after FunctionsConfig struct change)

68a66e5

Change JS user.save function params to (body, docID?)

74f73a3

per latest spec

JS delete fn takes either docID or body as param

273c710

- delete(docID) deletes the doc with that ID. - delete(body) uses body._id (required) and body._rev (optional). If _rev is given, uses MVCC.

Functions: Ensure only SELECT queries can be used

0cc7d0e

This isn't 100% reliable because it just uses a simple regex. A mutating query could be put after a SELECT and a semicolon. I think there might also be clauses that can validly go before SELECT (?)

Use conventional order of GraphQL resolver parameters

b094458

(parent, args, context, info) ... not (context, args, parent, info)

Updated function 'allow' pattern syntax

65bb5a8

Now uses ${...} and supports multiple path components and array indexing.

Implemented new mutability rules for functions/GraphQL

504cd0e

GraphQL top-level resolvers should not be allow-by-default

3548956

Field resolvers of the Query and Mutation types should treat a missing "allow" object as meaning admin-only, the way functions do.

WIP: V8

dfcedb9

WIP

94590af

Added TS fn+graphql code as db/functions/engine

b6597f4

Can call a function with V8!

19e71b6

Reimplementing functions package using V8

59de802

WIP Many unit tests passing

4e3cd5b

OMG all db/functions unit tests pass

adcaf3d

Cleanup: comments and making evaluator non-public

97f85f6

Made V8-based code thread-safe

b080edd

Added Apollo subgraph support. Fixed typename resolvers.

30aae2d

TypeScript cleanup and improved README

490a324

Refactored V8 code out of functionspackage

ceed13c

Refactored js API

2daa450

more refactoring

9e88cac

Reimplemented channels.ChannelMapper using js package & V8

9859c63

WIP: Nearly all SG unit tests pass

1b364c9

Limit number of simultaneous VMs a VMPool will create

2aa5273

Fixed: JS recursion-depth limit false positives

20ac4dc

Implemented timeouts for V8-based functions

f646fee

js.VMPool: stop using sync.Pool

05ba095

sync.Pool drops objects fairly often, and the VM objects and their services are pretty expensive to recreate; this was bad for performance.

torcolvin and others added 13 commits May 26, 2023 07:45

CBG-3024 Make sure CE import uses checkpoints (#6261)

c17d66c

* CBG-3024 Make sure import feed uses checkpoints

CBG-2977 allow DELETE on a broken DB config (#6260)

c471697

Make tests pass with default collections/views (#6267)

c7b78bf

CBG-3043: pick up cbgt fix for panic in import feed (#6270)

2940cb1

CBG-3028: fixes for failing CE tests (#6279)

7dff1d1

* CBG-3028: fixes for failing CE tests * remove print line * updates off comment

CBG-2857 Remove unambiguous timeouts from triggering cbcollections (#…

76f92e0

…6280)

Comment out windows v8 tests

153a365

Merge branch 'master' into feature/v8+subgraph

05282f6

make tags pass without v8

1de4377

Add v8 tests to jenkins (#6288)

d9a7b84

Moved the 'js' module to the sg-bucket repo

10a44bf

torcolvin reviewed Jun 6, 2023

View reviewed changes

snej added 2 commits June 7, 2023 16:02

Changes from Tor's review comments

c60ed61

Deep-copy doc bodies passed to Otto conflict handlers

0891f2e

Otto exposes Go objects directly to JS, meaning that the resolver fn is capable of modifying the document maps. A conflict resolver isn't allowed to do that, so make deep copies of both docs before calling it.

snej mentioned this pull request Jun 13, 2023

Added js module: new JavaScript API supporting V8 couchbase/sg-bucket#91

Open

snej added 4 commits June 20, 2023 14:01

Updated sg-bucket version in go.mod

4c6436b

Adapt to Context-related changes in js module

b4b458a

v8go package moved to couchbasedeps

b759efd

bbrks force-pushed the main branch from a9811cd to 1879730 Compare April 8, 2024 11:02

torcolvin unassigned torcolvin and adamcfraser Sep 25, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use V8 instead of Otto for JavaScript #5980

Use V8 instead of Otto for JavaScript #5980

snej commented Dec 16, 2022 •

edited

Loading

torcolvin left a comment

torcolvin Jun 6, 2023

snej Jun 7, 2023

torcolvin Jun 6, 2023

snej Jun 7, 2023

torcolvin Jun 6, 2023

snej Jun 7, 2023

torcolvin Jun 6, 2023

snej Jun 7, 2023

torcolvin Jun 6, 2023

snej Jun 7, 2023

torcolvin Jun 6, 2023

snej Jun 7, 2023

torcolvin Jun 6, 2023

snej Jun 7, 2023

torcolvin Jun 6, 2023

snej commented Jun 8, 2023

torcolvin commented Jun 8, 2023

gregns1 commented Jun 8, 2023

snej commented Jun 8, 2023

snej commented Jun 8, 2023

	return false, fmt.Errorf("import filter function returned non-boolean type %T", result)
	return false, fmt.Errorf("import filter function returned non-boolean type %T: %+v", result, result)

Use V8 instead of Otto for JavaScript #5980

Are you sure you want to change the base?

Use V8 instead of Otto for JavaScript #5980

Conversation

snej commented Dec 16, 2022 • edited Loading

Pre-review checklist

Dependencies (if applicable)

torcolvin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

snej commented Jun 8, 2023

torcolvin commented Jun 8, 2023

gregns1 commented Jun 8, 2023

snej commented Jun 8, 2023

snej commented Jun 8, 2023

snej commented Dec 16, 2022 •

edited

Loading