worker: add Locks API #22719

devsnek · 2018-09-05T23:24:41Z

References:
https://wicg.github.io/web-locks/
https://github.com/WICG/web-locks
https://github.com/WICG/web-locks/blob/master/EXPLAINER.md

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

nodejs-github-bot · 2018-09-05T23:24:42Z

@devsnek build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/808/pipeline

mscdex · 2018-09-05T23:41:40Z

src/env.h

-  V(write_host_object_string, "_writeHostObject")                             \
-  V(write_queue_size_string, "writeQueueSize")                                \
-  V(x_forwarded_string, "x-forwarded-for")                                    \
+#define PER_ISOLATE_STRING_PROPERTIES(V)                                       \


Can we make the spacing the same as before on all of these lines so that the diff will be much easier to view?

make format-cpp just kinda... did that. i'm not sure how to undo it.

They would have to be done manually. +1 on reverting this particular change.

mscdex · 2018-09-05T23:42:57Z

src/node_internals.h

-    V(v8)                                                                     \
-    V(worker)                                                                 \
-    V(zlib)
+#define NODE_BUILTIN_STANDARD_MODULES(V)                                       \


Ditto here about the spacing

TimothyGu

Some preliminary comments. /cc @addaleax as the go-to C++ person :)

TimothyGu · 2018-09-06T01:09:13Z

src/node_locks.cc

+// static
+LockManager* LockManager::GetCurrent() {
+  if (current_ == nullptr) {
+    current_ = new LockManager();


This is not thread-safe. What if multiple threads calls GetCurrent at the same time? I'd rather just initialize it for once in the main thread Environment's init method.

It doesn’t even have to be initialized with new (and if it is, we should use smart pointers for that) – it can be a LockManager LockManager::current_;, no pointers or anything involved

The alternative would be double locking this.

TimothyGu · 2018-09-06T01:09:55Z

src/node_locks.cc

+  LockManager* manager = LockManager::GetCurrent();
+
+  // Let queue be origin's lock request queue.
+  auto queue = &manager->pending_requests_;


Use explicit types and references. This applies to most usage of auto in this PR.

In particular, use references if that is what you are going for – these lines here actually create copies of pending_requests_ and held_locks_, but it’s not clear whether that’s intended?

TimothyGu · 2018-09-06T01:11:39Z

src/node_locks.cc

+
+    // Prepend request in origin's lock request queue.
+    std::unique_ptr<LockRequest> request{this};
+    queue->push_front(std::move(request));


emplace_front(this)

TimothyGu · 2018-09-06T01:13:01Z

src/node_locks.h

+
+ private:
+  Mode mode_;
+  const char* name_;


This has unclear data ownership. Use std::string instead.

TimothyGu · 2018-09-06T01:13:20Z

src/node_locks.h

+  node::Persistent<v8::Value> released_promise_;
+};
+
+class LockRequest : public v8::Task {


As discussed offline, there is no benefit to making this a v8::Task versus just a regular libuv-backed object as used elsewhere in Core.

I think there's a benefit of simplicity... I would still need a class to store this all and still need to insert it into the foreground queue somehow. Might as well use the API we have specifically for that right?

I agree with @TimothyGu here, this should ideally be a libuv task (see ThreadPoolWork in node_internals.h)

TimothyGu · 2018-09-06T01:14:04Z

src/node_locks.h

+
+ private:
+  Environment* env_;
+  const char* name_;


Ditto about strings.

targos · 2018-09-06T08:12:04Z

I edited the OP to add the reference links provided by @inexorabletash in #22702 (comment).

benjamingr · 2018-09-06T08:04:29Z

lib/internal/worker.js

@@ -505,12 +507,80 @@ function pipeWithoutWarning(source, dest) {
  dest._maxListeners = destMaxListeners;
 }

+class LockManager {
+  request(name, options, callback) {
+    if (callback === undefined) {


Nit, to differentiate this from a callback API can we call this ‘disposer’ and not ‘callback’

It's called "callback" in the spec.

I would prefer this to be consistent with Node.js terminology. A callback for Node.js is an error-back. This would be confusing. This would also apply to our docs.

I agree. While I generally prefer to keep naming the same or as same as possible to specs, in this case not calling this callback makes sense. Also, on the off chance that someone decided to try to wrap this in util.promisify() because it looks like a callback, it may make sense to have a custom promisify implementation for this.

util.promisify makes no sense here, it already returns a promise. i will rename the variable.

Yes, I know that util.promisify() makes no sense here, but that doesn't mean users won't try. Being defensive by providing a custom promisify implementation that skips wrapping and returns the actual promise makes sense... or, throw within the custom promisify to indicate that it doesn't make sense.

benjamingr · 2018-09-06T08:06:12Z

lib/internal/worker.js

+    if (name.startsWith('-')) {
+      // if name starts with U+002D HYPHEN-MINUS (-), then reject promise with a
+      // "NotSupportedError" DOMException.
+      reject(new DOMException('NotSupportedError'));


To guard against cases where we throw by accident can this be an async function and throw instead of reject here?

We can still do the new promise dance below

Also, not sure we should reject with a DOMException?

benjamingr · 2018-09-06T08:09:25Z

src/node_locks.cc

+// static
+LockManager* LockManager::GetCurrent() {
+  if (current_ == nullptr) {
+    current_ = new LockManager();


The alternative would be double locking this.

addaleax · 2018-09-06T08:00:45Z

src/node_locks.h

+
+  Lock(Environment* env,
+       Lock::Mode mode,
+       std::string name,


const std::string& (to avoid extra copies)

addaleax · 2018-09-06T08:01:09Z

src/node_locks.h

+  Mode mode_;
+  std::string name_;
+  node::Persistent<v8::Value> waiting_promise_;
+  node::Persistent<v8::Value> released_promise_;


The node:: namespacing can be dropped

addaleax · 2018-09-06T08:02:12Z

src/node_locks.h

+  LockRequest(Environment* env,
+              v8::Local<v8::Value> promise,
+              v8::Local<v8::Function> callback,
+              std::string name,


ditto about std::string here and in name() below

addaleax · 2018-09-06T08:02:39Z

src/node_locks.h

+    released_promise_.Reset();
+  }
+
+  inline std::string name() const { return name_; }


const std::string& (to avoid extra copies)

addaleax · 2018-09-06T08:04:26Z

src/node_locks.cc

@@ -0,0 +1,329 @@
+#include "node_locks.h"
+#include <memory>  // std::move


This seems odd, if you’re after std::move the header would be <utility>

addaleax · 2018-09-06T08:13:18Z

src/node_locks.cc

+                                   request->mode() == Lock::Mode::kShared
+                                       ? "shared"
+                                       : "exclusive"))
+          .ToChecked();


This seems to share a bunch of code with the block below … can we merge that to some degree?

addaleax · 2018-09-06T08:14:30Z

src/node_locks.cc

+              env->mode_string(),
+              FIXED_ONE_BYTE_STRING(
+                  env->isolate(),
+                  lock->mode() == Lock::Mode::kShared ? "shared" : "exclusive"))


I don’t think FIXED_ONE_BYTE_STRING would work properly when the argument is not a fixed string

addaleax · 2018-09-06T08:15:09Z

src/node_locks.cc

+  CHECK(args[5]->IsBoolean());
+
+  String::Utf8Value name(env->isolate(), args[2]);
+  Lock::Mode mode = (Lock::Mode)args[3].As<Number>()->Int32Value();


Use static_cast whenever possible

addaleax · 2018-09-06T08:18:20Z

src/node_locks.h

+  node::Persistent<v8::Value> released_promise_;
+};
+
+class LockRequest : public v8::Task {


I agree with @TimothyGu here, this should ideally be a libuv task (see ThreadPoolWork in node_internals.h)

addaleax · 2018-09-06T08:19:50Z

src/node_locks.h

+
+  void Snapshot(Environment* env, v8::Local<v8::Value> promise);
+  void ProcessQueue(Environment* env);
+  bool IsGrantable(LockRequest* request);


I think both the method and the LockRequest* parameter can be marked const?

mcollina

Good work so far

mcollina · 2018-09-06T08:37:19Z

lib/internal/worker.js

@@ -505,12 +507,80 @@ function pipeWithoutWarning(source, dest) {
  dest._maxListeners = destMaxListeners;
 }

+class LockManager {
+  request(name, options, callback) {
+    if (callback === undefined) {


I would prefer this to be consistent with Node.js terminology. A callback for Node.js is an error-back. This would be confusing. This would also apply to our docs.

jasnell · 2018-09-06T17:38:18Z

src/node_locks.cc

+
+// promise, callback, name, mode, ifAvailable, steal
+void LockManager::Request(const FunctionCallbackInfo<Value>& args) {
+  Environment* env = Environment::GetCurrent(args);


Likely better to push it to a separate follow up PR, but it would likely be helpful to emit trace events to track lock requests, grants, and releases to make them easier to debug.

jasnell · 2018-09-06T18:45:23Z

src/node_locks.cc

+  // If steal is true, then run these steps:
+  if (steal_) {
+    // For each lock of held:
+    for (auto const& lock : manager->held_locks_) {


@addaleax ... just thinking out loud here... would an std::unordered_map<std::string,std::deque<std::unique_ptr<Lock>> work better here to avoid having to loop through all locks and perform a string comparison on each?

That’s very much possible – I haven’t really looked at the PR on that level, but it sounds like something that makes sense to me.

jasnell · 2018-09-06T18:48:50Z

src/node_locks.cc

+              ->Call(env_->context(), Undefined(env_->isolate()), 1, &null)
+              .ToLocal(&r)) {
+        // Resolve promise with r and abort these steps.
+        promise->Resolve(env_->context(), r).ToChecked();


@addaleax ... does this function need to include an InternalCallbackScope?

I would say so, yes… That would also mean that this class should inherit from AsyncWrap as well (but that makes sense, since it does run JS code asynchronously), and ditto for the other place you just commented

jasnell · 2018-09-06T18:53:15Z

src/node_locks.cc

+      if (request->callback()
+              ->Call(env->context(), Undefined(env->isolate()), 1, args)
+              .ToLocal(&r)) {
+        waiting->Resolve(env->context(), r).ToChecked();


@addaleax ... in this function too... is InternalCallbackScope required?

devsnek · 2018-09-07T02:27:40Z

@jasnell is this the correct usage of the trace api?

            // When lock lock's waiting promise settles (fulfills or rejects),
            // enqueue the following steps on the lock task queue:
            waitingPromise
              .finally(() => undefined)
              .then(() => {
                // Release the lock lock.
                release();

                trace('e'.charCodeAt(0), 'node.locks', 'LOCK_RELEASE', 0, { name });

                // Resolve lock's released promise with lock's waiting promise.
                resolve(waitingPromise);
              });

TimothyGu · 2018-09-07T04:25:26Z

src/node_locks.cc

+
+      // Remove request from queue.
+      pending_requests_.erase(
+          std::remove(


std::remove is C++17, which we can't use.

addaleax · 2018-09-16T16:18:08Z

@devsnek What’s the status here?

devsnek · 2018-09-16T16:21:58Z

@addaleax currently rewriting this to fix some issues... timeline unknown

bnoordhuis · 2018-12-06T23:04:34Z

@devsnek Any updates? It looks to be bitrotting pretty bad.

devsnek · 2018-12-06T23:07:03Z

I'd like to get back to this eventually unless someone else wants to take it up. I'll close this pr for now since it's pretty much useless at this point.

This is based upon nodejs#22719 and exposes the same API. Instead of a C++-backed approach, this variant implements the API mostly in JS. Refs: nodejs#22719 Co-authored-by: Gus Caplan <[email protected]>

devsnek added wip worker labels Sep 5, 2018

devsnek requested review from mcollina, jasnell, addaleax and benjamingr September 5, 2018 23:24

nodejs-github-bot added c++ lib / src labels Sep 5, 2018

devsnek force-pushed the feature/locks-api branch from 30f95f6 to 7c06fe4 Compare September 5, 2018 23:29

mscdex reviewed Sep 5, 2018

View reviewed changes

devsnek force-pushed the feature/locks-api branch from 7c06fe4 to 6099150 Compare September 5, 2018 23:43

TimothyGu reviewed Sep 6, 2018

View reviewed changes

benjamingr reviewed Sep 6, 2018

View reviewed changes

addaleax reviewed Sep 6, 2018

View reviewed changes

mcollina reviewed Sep 6, 2018

View reviewed changes

devsnek force-pushed the feature/locks-api branch from 48a1307 to 37ec99d Compare September 6, 2018 17:14

jasnell reviewed Sep 6, 2018

View reviewed changes

devsnek force-pushed the feature/locks-api branch 3 times, most recently from 3aed3ef to 81eba51 Compare September 7, 2018 01:22

worker: implement web locks api

44532ff

devsnek force-pushed the feature/locks-api branch from 81eba51 to 44532ff Compare September 7, 2018 01:50

squash! properly enter microtask queue

1626f59

squash! fix default option

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.

GPG key ID: 4AEE18F83AFDEB23
Expired

Verified
Learn about vigilant mode

43ff4d3

TimothyGu reviewed Sep 7, 2018

View reviewed changes

src/node_locks.cc

// Remove request from queue.

pending_requests_.erase(

std::remove(

Copy link

Member

TimothyGu Sep 7, 2018

Choose a reason for hiding this comment

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

std::remove is C++17, which we can't use.

squash! if locks are pending on exit, there is a deadlock

27f5d56

addaleax added the semver-minor label Sep 16, 2018

devsnek closed this Dec 6, 2018

addaleax mentioned this pull request Dec 13, 2020

worker: implement Web Locks API #36502

Open

4 tasks

		@@ -0,0 +1,329 @@
		#include "node_locks.h"
		#include <memory> // std::move

worker: add Locks API #22719

worker: add Locks API #22719

Conversation

devsnek commented Sep 5, 2018 • edited Loading

Checklist

nodejs-github-bot commented Sep 5, 2018

Choose a reason for hiding this comment

devsnek Sep 5, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TimothyGu 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

targos commented Sep 6, 2018

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcollina 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

devsnek commented Sep 7, 2018

Choose a reason for hiding this comment

addaleax commented Sep 16, 2018

devsnek commented Sep 16, 2018

bnoordhuis commented Dec 6, 2018

devsnek commented Dec 6, 2018

devsnek commented Sep 5, 2018 •

edited

Loading

devsnek Sep 5, 2018 •

edited

Loading