Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WIP: Integrate Monitor into a @monitored context #2

Open
wants to merge 1 commit into
base: staging
Choose a base branch
from

Conversation

CMCDragonkai
Copy link
Member

@CMCDragonkai CMCDragonkai commented Jun 15, 2023

Description

This will make it easier to use the Monitor in various class methods such as in `js-quic.

This creates a HOF called monitored as well as class method decorator called @monitored.

We can imagine a class like this:

class X {
  public f(ctx?: Partial<ContextMonitoredInput>): Promise<void>;
  @monitored()
  public async f(@context ctx: ContextMonitored): Promise<void> {
    await ctx.monitor.lock('a'); // Mutual exclusion indexed by `a`
  }

  public g(ctx?: Partial<ContextMonitoredInput>): Promise<void>;
  @monitored()
  public async g(@context ctx: ContextMonitored): Promise<void> {
    await ctx.monitor.lock('a'); // Mutual exclusion indexed by `a`
  }
}

And equivalent for the monitored HOF.

There are several decision decisions we have to make here.

  1. Where does the LockBox<RWLock> stay? Consider that the @monitored would require the ability to point to a shared lock box object. We can imagine a few places where it can exist. The first place would as a symbol property of the instantiated class. This is doable just through an additional class decorator, but it is also possible to lazily instantiate this just through the @monitored() decorator call. Furthermore it should also be possible to inject a lock box at that call in case the @monitored decorator can use separate share lockboxes.
  2. At the same time when creating the lockbox, one must also choose the appropriate lockConstructor: new () => RWLock. I think this can be defaulted to RWLockWriter, in which case the lockBox defaults to LockBox<RWLockWriter>.
  3. Finally for deadlock detection, it's optional, so again this can be something that is part of the class object properties through a symbol (unique symbols prevent name clashes with the underlying class properties), but again it could be something that can be specified. I think the default behaviour would be not to specify it, because the default Monitor also doesn't specify it, and it does slow down the monitor usage, and it's best used during prototyping and testing, and once it's confirmed that no deadlocks are possible, it can be dropped from the production code.
  4. The @monitored could imply @cancellable due to the existence of PromiseCancellable<void>. Furthermore, we support timers in our Monitor.lock method which means actually using @monitored could imply @timedCancellable too.
  5. Now if @timed is automatic for @monitored, what does this actually mean? Especially for calls to ctx.monitor.lock()? Normally @timed provides both a ctx.signal and ctx.timer, however it does not do an automatic rejection like @cancellable does. When we are doing @monitored, we may not actually want to be able to do automatic rejection, it feels like a separate thing.
  6. Therefore it is a separate thing, then one could expect @monitored to be combined with @timedCancellable, and the order of wrapping does matter here too. Is it @monitored first and then @timedCancellable second or the other way around? Furthermore is there efficiency gained from combining it into a single decorator like @monitoredTimedCancellable or @timedCancellableMonitored?
  7. If @timed or @cancellable is combined with @monitored, then it must be that the ctx must automatically be passed into any ctx.monitor.lock() or ctx.monitor.withF or ctx.monitor.waitForUnlock... etc calls. This is because there's an expectation that ctx is passed down the call graph, but one could easily forget to do this when using the ctx.monitor.lock call. And it just seems that if I wanted to either time out the entire operation, then it should time out any locking operations, and if I want to cancel the whole call, then it also makes sense that I would want to cancel any locking operations.
  8. What is the API of ctx.monitor, is the full Monitor API? If so, then one has to actually do await ctx.monitor.lock('a')() or use it as part of the withF(ctx.monitor.lock('a'), async () => { ... }. This isn't that bad, but it does require a little more effort then the DBTransaction.lock call. The main reason is to align the API similar to other resources.
  9. When @monitored is used, there needs to be a way to "construct" a monitor from scratch. For example in @timed, one can create a new Timer, and for @cancellable, one can create a new AbortController, but one cannot create a new Monitor without being able to refer to the shared lock box some how. I think if the lock box was exposed through a symbol property like obj[lockBox], then it would be possible to construct a new monitor like obj.method({ monitor: new Monitor(obj[lockBox], obj[lockConstructor], obj[locksPending]) }. The only issue here is that there's automatic destruction of the monitor, if you do this, you need to close the monitor somehow at the end with await monitor.unlockAll(). This is usually why we have a convenience function called withF() or withMonitorF or withTransactionF such as in the DB. This isn't necessary for the Timer or AbortController, because they can be garbage collected, but now I realise it does actually make sense to do so... to get around this it may make sense for the user to provide a method to create these resources and then combine it with withF. Even the Timer requires automatic cancellation...
class X {
  public f(ctx?: Partial<ContextMonitoredInput>): Promise<void>;
  @monitored()
  public async f(@context ctx: ContextMonitored): Promise<void> {
    console.log(ctx.monitor);
    await ctx.monitor.lock('a');
  }
}

const x = new X();

Right now this what you'd do similar to new Timer or new AbortController.

// Assume you needed to create a fresh monitor from scratch
const monitor = new Monitor(x[lockBox], x[lockConstructor]);
await x.f({ monitor });
// You need to remember to clear it yourself
await monitor.unlockAll();

Alternatively:

// Convenience bracket-pattern function for creating a monitor resource
function monitor(): ResourceAcquire<Monitor<RWLock>> {
  return async () => {
    const monitor = new Monitor(x[lockBox], x[lockConstructor]);
    return [
      async () => {
        await monitor.unlockAll();
      },
      monitor
    ];
  };
}

// Now we can use this instead
await withF(monitor(), async ([mon]) => {
  await x.f({ monitor: mon });
});

Alternatively:

// What if we had the convenience function embedded in the object?
await x.withMonitorF(async ([mon]) => {
  await x.f({ monitor: mon });
});

// And you could also call it directly
const mon =  x.monitor();

To prevent nameclashes, you'd have to use a symbol method:

import { monitor, withMonitorF, withMonitorG }

await x[withMonitorF](async ([mon]) => {
  // ...
});

await x[withMonitorG](async ([mon]) => {
  // ...
});

const mon =  x[monitor]();

Note that this pattern is called a "mix-in" pattern. We are mixing-in the ability to create specific resources and bracketing methods for the object. This is primarily useful when we expect the "unit of atomicity" to be any method with the @monitored decorator applied. However if the unit of atomicity is expanded beyond one single class, then you wouldn't want to use those mixins, since they imply a single-class unit. This is why @monitored should not automatically mixin those methods. There should also be a class decorator that supports the mixins if you want.

It might look like:

// Class decorators are always capitalised
@Monitored()
class X {
  public f(ctx?: Partial<ContextMonitoredInput>): Promise<void>;
  @monitored()
  public async f(@context ctx: ContextMonitored): Promise<void> {
    console.log(ctx.monitor);
    await ctx.monitor.lock('a');
  }
}

All of it does add a bit of verbosity though.

So if we were to combine @timedCancellable, it seems that it would make sense to decorate @timedCancellable first. I think the order of decorators mathematical.

@Monitored()
class X {
  public f(ctx?: Partial<ContextMonitoredInput>): Promise<void>;
  @timedCancellable()
  @monitored()
  public async f(@context ctx: ContextMonitored): Promise<void> {
    console.log(ctx.monitor);
    await ctx.monitor.lock('a');
  }
}

This should create a situation where the @timedCancellable is the last wrapper, and returns a function that will take ContextTimedInput. This will produce ContextTimed internally. Then when monitored function takes the ContextTimed, it can make use of the ctx.timer and ctx.signal and autopropagate them into the ctx.monitor.lock() calls.

That would require:

type ContextMonitoredInput = {
  monitor: Monitor<RWLock>;
  signal?: AbortSignal;
  timer?: Timer | number;
}

So with the signal and timer optional, the idea is that these will be auto sent in.

Alternatively we can not bother with automatic injection, less magic, and just expect the user to pass the ctx into ctx.monitor.lock call. Something like:

@Monitored()
class X {
  public f(ctx?: Partial<ContextMonitoredInput>): Promise<void>;
  @timedCancellable()
  @monitored()
  public async f(@context ctx: ContextMonitored): Promise<void> {
    console.log(ctx.monitor);
    await ctx.monitor.lock('a', ctx);
  }
}

Of course we should also upgrade our existing setup* functions to support the literal timer as a number as we have done so in js-async-locks.

There's quite a few things here to explore...

Issues Fixed

Tasks

  • 1. Install this into js-contexts
  • 2. Test out @monitored decorator and monitored HOF in js-contexts
  • 3. Bring in the literal timer option as we have in js-async-locks, it should make timer usage a bit easier too

Final checklist

  • Domain specific tests
  • Full tests
  • Updated inline-comment documentation
  • Lint fixd
  • Squash and rebased
  • Sanity check the final build

@CMCDragonkai
Copy link
Member Author

CMCDragonkai commented Jun 15, 2023

This brings the 4.0.0 of @matrixai/async-locks. Other code will need to update the async-locks usage in particular async-init to ensure things are still working fine. If they don't have any major breaking API, then it should be great.

@ghost
Copy link

ghost commented Jun 15, 2023

👇 Click on the image for a new way to code review

Review these changes using an interactive CodeSee Map

Legend

CodeSee Map legend

@CMCDragonkai
Copy link
Member Author

Note that having this eventually means that DBTransaction is sort of abstracted too, as it can eventually just use the Monitor directly.

@CMCDragonkai
Copy link
Member Author

This creates utils.monitor and utils.timer as ResourceAcquire functions for later composition into any kind of convenience creation if withTimerF or withMonitorF when used with @timed or @timedCancellable or @monitored decorators.

@CMCDragonkai CMCDragonkai self-assigned this Jun 18, 2023
@CMCDragonkai
Copy link
Member Author

CMCDragonkai commented Jun 19, 2023

@tegefaulkes I think given the complexity of this problem, I'm thinking that js-quic should just move ahead with directly using the Monitor.

It would work the same as how DBTransaction works, where the Monitor instances are created for each method call in QUICConnection while the shared lock box is within each QUICConnection instance. No deadlock detection of course since we would not expect it to occur anyway.

It does mean calls should take the mon?: Monitor as the last parameter and do things like:

if (mon == null) {
  return this.withMonitorF((mon) => this.f(mon));
}

Like we currently do with tran?: DBTransaction.

Then when this is perfected, we can migrate towards that in the future.

@CMCDragonkai
Copy link
Member Author

We will leave this till later. We only need to cherry pick the changes that enable a literal timer into staging and release a new js-contexts.

@CMCDragonkai
Copy link
Member Author

Literal timer logic extracted out, will be merged in #3. This has been rebased on top of staging.

@CMCDragonkai
Copy link
Member Author

Some design notes regarding transaction decorator syntax too that is related to how we expect monitors to work: MatrixAI/js-db#51

@CMCDragonkai
Copy link
Member Author

Staging now has the monitor resource. That should be used by js-quic and perhaps js-ws for controlling call-graph mutual exclusion and enabling re-entrancy.

It's a little inefficient though to always generating a new monitor on every root of mutually exclusive callgraphs. I'm not sure if this really can be solved right now, one way might be to optimise the creation of the monitor as much as possible, since they need to be independent in order to enforce mutual exclusion.

The selection of the key should really be a constant though @tegefaulkes, rather than a instance property. Such keys could be used in the utils.ts.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

1 participant