Diagnostics/Debuggability/Management

[inexorabletash]

Potentially outside the bounds of the spec, but all implementations will need this e.g. for console integration. Perhaps it should be part of the API:

• ability to enumerate held flags for an origin
• ability to forcibly "break locks"

As is noted for Indexed DB the ability for an origin to get a snapshot of the state of things for diagnostic or cleanup purposes is desired.

Plausible API:

navigator.flags.request(scope, mode, options); // instead of requestFlag
navigator.flags.inspect(); // returns [{scope, mode}, ...] - state of held flags across all context
navigator.flags.releaseAll(); // forcibly releases all held flags and aborts all flag requests

The last option is intentionally terrifying so as to discourage more targeted use; it should only be a last resort.

WDYT?

[inexorabletash]

Obviously, if there's an API to forcibly release it means that an origin needs to account for a held flag suddenly being released. (flag.released would resolve suddenly, is that sufficient?)

[jakearchibald]

These APIs will have to be async right?

[inexorabletash]

These APIs will have to be async right?

Yes.

[inexorabletash]

A note from potential users that releaseAll() is too broad. Targetting a subset of resources will be necessary, so maybe locks.forceRelease(name or [names...])

[inexorabletash]

partial interface Navigator {
  readonly attribute LockManager locks;
};

interface LockManager {
  Promise<Lock> acquire((DOMString or sequence<DOMString>) scope,
                        optional LockOptions options);

  Promise<sequence<LockStatus>> inspect();
  void forceRelease((DOMString or sequence<DOMString>) scope);
};

dictionary LockStatus {
  DOMString name;
  LockMode mode;
};

WDYT?

[ralphch0]

API looks good to me.
Only concern is "inspect" giving a "dev tools" connotation, and suggesting that the API should not be used in prod code. There is an 'inspect' function for the command line: https://developers.google.com/web/tools/chrome-devtools/console/command-line-reference#inspect

Also, even though it might be obvious, it doesn't specify that we're returning only locks that are "taken".

I don't have suggestions that are much better. Been thinking about getAllAcquired() ?

[inexorabletash]

Good point re: “inspect” as a name. Also... for full fidelity it should probably return both held locks *and* queued requests.

…

On Nov 2, 2017, at 2:45 PM, ralphch0 ***@***.***> wrote: API looks good to me. Only concern is "inspect" giving a "dev tools" connotation, and suggesting that the API should not be used in prod code. There is an 'inspect' function for the command line: https://developers.google.com/web/tools/chrome-devtools/console/command-line-reference#inspect Also, even though it might be obvious, it doesn't specify that we're returning only locks that are "taken". I don't have suggestions that are much better. Been thinking about getAllAcquired() ? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub, or mute the thread.

[inexorabletash]

partial interface Navigator {
  readonly attribute LockManager locks;
};

interface LockManager {
  Promise<Lock> acquire((DOMString or sequence<DOMString>) scope,
                        optional LockOptions options);

  Promise<LockState> queryState();
  void forceRelease((DOMString or sequence<DOMString>) scope);
};

dictionary LockState {
  held sequence<LockStatus>;
  requested sequence<LockStatus>;
};

dictionary LockStatus {
  sequence<DOMString> scope;
  LockMode mode;
};

[ralphch0]

LockStatus doesn't actually contain a status though, right? Maybe LockStatus -> LockRequest?
Being able to inspect the requested resources could be interesting to report on lock request contention.

[inexorabletash]

Maybe LockStatus -> LockRequest?

sgtm! (Dictionary names aren't exposed to script so we don't have to stress about those names. But naming things is hard so I appreciate any/all suggestions.)

Being able to inspect the requested resources could be interesting to report on lock request contention.

Exactly - this came up enough with Indexed DB that I think we should built it in from the start here.

partial interface Navigator {
  readonly attribute LockManager locks;
};

interface LockManager {
  Promise<Lock> acquire((DOMString or sequence<DOMString>) scope,
                        optional LockOptions options);

  Promise<LockState> queryState();
  void forceRelease((DOMString or sequence<DOMString>) scope);
};

dictionary LockState {
  held sequence<LockRequest>;
  requested sequence<LockRequest>;
};

dictionary LockRequest {
  sequence<DOMString> scope;
  LockMode mode;
};

This state is pretty much exactly what I have as debug spew in my implementation when I was debugging it and writing the tests, which tells me it would be useful for consumers of the API.

https://chromium-review.googlesource.com/c/chromium/src/+/657902/15/content/browser/locks/locks_context.cc#222

[ralphch0]

Also, requested -> "pending" or "waiting" or "blocked" or "queuing" ?
Could be nitting here, but held locks are technically also "requested".

We should define the order of this field? i.e. in ascending order of request time ?
I don't know where to draw the line in terms of what we expose.. should we expose request timestamp?

[inexorabletash]

"pending" sgtm

partial interface Navigator {
  readonly attribute LockManager locks;
};

interface LockManager {
  Promise<Lock> acquire((DOMString or sequence<DOMString>) scope,
                        optional LockOptions options);

  Promise<LockState> queryState();
  void forceRelease((DOMString or sequence<DOMString>) scope);
};

dictionary LockState {
  held sequence<LockRequest>;
  pending sequence<LockRequest>;
};

dictionary LockRequest {
  sequence<DOMString> scope;
  LockMode mode;
};

We should define the order of this field? i.e. in ascending order of request time ?

Yeah, would be ordered. The handwavy proto spec defines an ordering of the request queue, presumably it would just dump a snapshot of that. The ordering of held locks is not defined, but could be for the purposes of this.

The order would match when the lock request hit the coordinator; this may not match wall-clock ordering across tabs as seen by the requesting code if e.g. IPC between a renderer/browser process (in Chrome terms) is delayed.

[inexorabletash]

I reworked the proto-spec so that "held locks" is a per-origin set; easy-peasy.

[ralphch0]

LGTM

[domenic]

Overall it seems like a reasonable thing to consolidate these methods into the LockManager. However, I wish I had a more complete picture of the IDL to judge the API by. What is LockOptions? What is LockMode? What is Lock's API?

Should you be able to forceRelease just by knowing the string name, or should you need to keep a reference to the Lock object?

I preferred inspect() to queryState(). I don't really think we can reserve that word for dev tools across the entire platform; it's a good word for introspection APIs. Maybe just query() would be OK.

Does inspect/queryState/query have any use cases besides releasing all locks? If so it seems like a bad fit given the async-ness.

We lost the ability since the OP to release all locks. Was that intentional, that you always need to keep track of the strings in use? I wasn't clear.

[inexorabletash]

However, I wish I had a more complete picture of the IDL to judge the API by.

These should help:

https://github.com/inexorabletash/web-locks/blob/master/interface.webidl
https://github.com/inexorabletash/web-locks/blob/master/proto-spec.md

Should you be able to forceRelease just by knowing the string name, or should you need to keep a reference to the Lock object?

The intent is that some sort of monitor service can reset an origin's state if it thinks it's detected a bug, so it would not be holding the lock. @ralphch0 may be able to provide more context.

Maybe just query() would be OK.

Noted. I'll leave a longer, obnoxious name in place while gathering more feedback.

Does inspect/queryState/query have any use cases besides releasing all locks? If so it seems like a bad fit given the async-ness.

The scenario came up with Indexed DB as developers - both locally and using reports "in the field" -
were trying to understand the behavior of what they'd built. e.g. not realizing that they were holding a connection in another tab, or what order things had occurred in. Being able to dump a snapshot of this hidden state is really useful, even if you can't use it for application logic.

Was that intentional, that you always need to keep track of the strings in use? I wasn't clear.

Based on @ralphch0's comment, targeted behavior is desirable. Ostensibly, an app should know the names it cares about. The inspection API allows for full enumeration in the worst case, and this is less critical than e.g. in the IndexedDB case since there's no persistent state or (worse) quota footprint that obsolete resources are consuming.

[inexorabletash]

Thanks for the questions, of course! This should all make it into the explainer and eventual spec ASAP.

[pwnall]

1. I would prefer that this is not encouraged for production use, and remains skewed towards diagnostics. I'd rather not have implementations worry about the speed at which they iterate through their internal state to produce a snapshot.

2. Including lock requests will make the spec more complicated -- how do we spec when you're guaranteed to "see" requests coming from other processes? If we need to go down this path (agreed that it could turn out very useful for diagnostics / debugging), I expect that we'll have to say "no guarantees" in the spec, like in API to enumerate databases IndexedDB#31

[inexorabletash]

I updated the proto-spec to use query(). For discussion about stealing vs. forced releasing there's more context in #23 so closing this out for now.