Use V8 instead of Otto for JavaScript #5980
Conversation
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-2983 Close cbgt agents on database close Manager.Stop() doesn’t close cbgt’s gocb agents - partly because the fts use case is to have one manager that exists for the lifetime of the process. For SG’s usage, where manager lifecycle is bound to a database, we need to shut down these agents when we close the database/importListener. * Improve inline documentation for CloseStatsClients call
* 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.
…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
| On a clean build, or after any changes in the `engine` source tree: | ||
|
|
||
| 1. `cd db/functions/engine` | ||
| 2. `npm run build` |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
That could be good, but I myself am not good at CI stuff so I don't trust myself to write it.
| func init() { | ||
| underscore.Disable() // It really slows down unit tests (by making otto.New take a lot longer) | ||
| } |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.)
db/crud.go
Outdated
| 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??*/) |
There was a problem hiding this comment.
Ah, right. false is correct, so I'll remove the comment.
| return func(ctx context.Context, doc Body) (bool, error) { | ||
| if timeout > 0 { | ||
| var cancelFn context.CancelFunc | ||
| ctx, cancelFn = context.WithTimeout(ctx, timeout) |
There was a problem hiding this comment.
otto doesn't use context cancellation, is this being obeyed in otto?
There was a problem hiding this comment.
Yup, see the other comment above (it's handled in js.OttoRunner.)
| case bool: | ||
| return result, nil | ||
| case string: | ||
| return strconv.ParseBool(result) |
There was a problem hiding this comment.
What's the justification for dropping this error handling?
There was a problem hiding this comment.
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.
db/import.go
Outdated
| case string: | ||
| return strconv.ParseBool(result) | ||
| } | ||
| return false, fmt.Errorf("import filter function returned non-boolean type %T", result) |
There was a problem hiding this comment.
| 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) |
rest/config.go
Outdated
| 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 { |
There was a problem hiding this comment.
you allow 999 args above, and this makes me think you want to put some constants for minJSArgs, maxImportFilterArguments
There was a problem hiding this comment.
It looks like an import filter only takes one arg, so I'll change the 999 and the 99 to 1.
| if err != nil { | ||
| return "", nil, err | ||
| } | ||
| localDocBody := localDoc.Body().ShallowCopy() |
There was a problem hiding this comment.
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.
|
Hm, got a single test failure: in "test v8 (ubuntu) (pull_request)" the test TestReconnectReplicator failed: |
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. |
|
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 |
|
The test log says |
- 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.
CBG-2949
Replaces Otto with V8 for all uses of JavaScript.
The new
jspackage is a general purpose thread-safe library for creating "services" that call a JavaScript function in an isolated runtime context.Pre-review checklist
fmt.Print,log.Print, ...)base.UD(docID),base.MD(dbName))docs/apiDependencies (if applicable)
Integration Tests
GSI=true,xattrs=truehttps://jenkins.sgwdev.com/job/SyncGateway-Integration/000/