Events Refactoring (integrating js-events)

[CMCDragonkai]

Description

https://github.com/MatrixAI/js-events provides a Evented decorator and EventAny that allows one to depend on all relevant of a given object.

This results in:

1. No longer single inheritance of EventTarget, instead we use a mixin.
2. Classes aren't supposed to extend EventTarget anyway
3. Encapsulated evented systems can have all their events listened for by just doing addEventListener(callback) without any type key string.
4. The declaration of events should now be Event... basically starting with an Event prefix similar to Error... prefix for errors.
5. The names of errors should statically checked by relying on the EventClass.name static property, rather than having to make up event names

Using events can be quite tricky, but we have come up with a general pattern.

1. EventXError - this is the general domain error event, all relevant exceptions will be part of the detail type.
2. EventXClose - this is a "close" event, the exact meaning depends on the construct. For example QUICStream has 2 separate close events for each side of the stream. On QUICConnection the close event handle has many duties because closing is an asynchronous concurrent process that involves draining whatever is still happening. The detail property will contain a subset of what is in the detail property of the error. This is because the error handler eats up some errors.
3. Each domain has their own handlers for both of those events as handleEventXError and handleEventXClose.
4. The error handler chain dispatches the close event.
5. The error handler will throw up any ErrorXInternal representing unrecoverable exceptions. This is expected to fail the program. Users can see it by assigning an uncaught exception handler. No close event is dispatched in this case.
6. The close handler will end up calling stop or destroy. But it does this intelligently, and just beforehand, it will resolve some promise indicating that closing is finished, that promise can be what the stop or destroy is waiting on.
7. When a user calls stop or destroy, this flow is separate from error -> close, instead the stop or destroy may proceed to dispatch close or error (even in case of graceful errors depending on the situation).
8. Some calls when hitting a "legitimate" error transition will dispatch the error event, but will continue. When hitting a non-legitimate error transition, will dispatch an internal error and then also throw up the error thus becoming a caller error.
9. In some cases, the close event can be dispatched independently, if a graceful close has nothing to do with an error event. In the case of QUICConnection, closing always involves an error even if the error code is 0 that's why graceful close is still an error event. But this is not necessarily true in other cases.
10. Encapsulated domains should have their events completely captured and redispatched upwards. This can be done with EventAll and handleEventX handlers. Injected domains should not have all their events completely captured and redispatched.
11. It is normal for parent domains to listen on child event domains.
12. Async things like this often have mutual calls, break up these mutual calls by identifying the dominant control flow origin (or origin of change), and have that be pull-based, while have the opposite flow be push-based. So you get a sort of pull-push hybrid. The primary advantage of this is to ensure that error handling is maintained within the relevant domain, instead of duplicating error handling logic. Sometimes this is not possible to break up, and you do need to duplicate it. In that case, consider factoring out the commonality if suitable, OR combine the 2 mutual parts into a single unit if suitable, and lacking all other options, duplicate the logic, but always identify the original (root) caller where the error handling happens, and where the buck has to stop otherwise resulting in uncaught exceptions (unless it is an unrecoverable error).
13. Unrecoverable errors can be implicit or explicit. An implicit unrecoverable error comes from calling a function that must succeed, and has no indication of throwing an error. An explicit unrecoverable error is where you call a function that may throw an exception, but it should not throw an exception except where there's a programmer error due to the logic, so you catch it and rethrow it upwards as an ErrorXInternal.

Issues Fixed

• Related to Consolidating Configuration Entropy and Nodes and Client Services Refactoring Polykey#552
• Related Integrate Observables to coordinate push-based Dataflow (with reactive properties) Polykey#444
• Related Extract src/websockets from Polykey to js-ws js-ws#5
• Fixes Streams not gracefully ending during high usage #20

Tasks

• 1. Replace all events with Event... prefixed class names.
• 2. Use @Evented() as class decorator for all things that use extends EventTarget - the js-async-init 1.9.x is now integrated with js-events
• 3. Use addEventListener(callback) instead of hardcoding all the relevant event names - Can use EventDefault.name instead.
• 4. Encapsulated evented objects should have all their events captured and handled, the ones it cannot handle needs to be re-emitted
• 5. The events.ts isn't properly setting up the event class hierarchy, they should be extending the abstract classes for the grouped events.
• 6. Update to node v20 (including pkgs.nix, shell.nix, scripts for win and mac)
• 7. Make sure that startup and shutdown of QUIC connections have no race conditions.
• 8. Create 3 new issues from this:
  - Native multithreading for performance - need to bench this against socket buffer size increase
  - Connection Migration - should support it for the client, but not sure for serer
  - Server name check should support IP checks if the server name is just the IP - requires upstream fix - Events Refactoring (integrating js-events) #53 (comment)
• 9. Test build with 0.18.0 (we are currently 2 versions behind)
  - Note this requires an update to boring to v3. There are some notes on v3 fixing the windows build problem. Failure when compiling boring-sys v2.1.0 on Windows cloudflare/boring#121 (comment) (So it's possible that 0.18.0 will be the build where windows succeeds!!)
• 10. Renamed toCanonicalIp to toCanonicalIP
• 11. Aligned generateCertificate with PK's x509 module where we actually setup the proper subject and issuer common names along with SAN for testing localhost DNS and 127.0.0.1 and ::1 IP.
• 12. Event flows from "error" to "close" to "stop", where recoverable errors results in an error event. But unrecoverable errors also go to "error" event that is then thrown upwards to result in EventError which by default results in an uncaught exception that represents a fail fast software error.
  -
• 13. TLS errors are now properly explained using the CryptoError enum. Note that this is unique to QUIC, where TLS 1.3 alert codes is offset by 0x0100. Node's https doesn't really expose the alert codes, it just abstracts over it.

Prototyping test.ts for random tests.

import Logger, { LogLevel, StreamHandler, formatting } from '@matrixai/logger';
import { CryptoError } from './src/native';
import QUICClient from './src/QUICClient';
import QUICServer from './src/QUICServer';
import * as events from './src/events';
import * as utils from './src/utils';
import * as testsUtils from './tests/utils';

// process.on('uncaughtExceptionMonitor', (err, origin) => {
//   console.log('Caught exception:', err);
//   console.log('Exception origin:', origin);
// });

// process.on('unhandledRejection', (reason, promise) => {
//   console.log('Unhandled Rejection at:', promise, 'reason:', reason);
// });

// IT"S AN UNCAUGHT EXCEPTION
process.on('uncaughtException', (err, origin) => {
  console.log('Caught exception:', err);
  console.log('Exception origin:', origin);
});

async function main() {
  const logger = new Logger(`stream_1KiB Bench`, LogLevel.INFO, [
    new StreamHandler(
      formatting.format`${formatting.level}:${formatting.keys}:${formatting.msg}`,
      // formatting.format`${formatting.date}-${formatting.level}:${formatting.keys}:${formatting.msg}`,
    ),
  ]);
  const data1KiB = Buffer.alloc(1024);
  const tlsConfig = await testsUtils.generateTLSConfig('RSA');
  const quicServer = new QUICServer({
    config: {
      verifyPeer: false,
      key: tlsConfig.leafKeyPairPEM.privateKey,
      cert: tlsConfig.leafCertPEM,
      ca: tlsConfig.caCertPEM
    },
    crypto: {
      key: await testsUtils.generateKeyHMAC(),
      ops: {
        sign: testsUtils.signHMAC,
        verify: testsUtils.verifyHMAC,
      },
    },
    logger: logger.getChild('QUICServer'),
  });
  quicServer.addEventListener(
    events.EventQUICServerConnection.name,
    // @ts-ignore
    (evt: events.EventQUICServerConnection) => {
      const connection = evt.detail;
      connection.addEventListener(
        events.EventQUICConnectionStream.name,
        // @ts-ignore
        async (evt: events.EventQUICConnectionStream) => {
          const stream = evt.detail;
          process.stderr.write('>>>>>>>>> HANDLING THE QUIC SERVER STREAM\n');
          await stream.writable.abort();
          // try {
          //   await stream.writable.close();
          // } catch (e) {
          //   console.log('FROM CLOSING WRITABLE', e.name, e.message);
          // }
          // Consume until graceful close of readable
          try {
            for await (const _ of stream.readable) {
              // Do nothing, only consume
            }
          } catch (e) {
            console.log('FROM READING STREAM', e.name, e.message);
          }
          process.stderr.write('<<<<<<<< HANDLED THE QUIC SERVER STREAM\n');
        }
      );
    }
  );

  await quicServer.start();

  let quicClient: QUICClient;
  try {
    const host = utils.resolvesZeroIP(quicServer.host);
    quicClient = await QUICClient.createQUICClient({
      host: host,
      port: quicServer.port,
      config: {
        verifyPeer: true,
        ca: tlsConfig.caCertPEM
      },
      crypto: {
        ops: {
          randomBytes: testsUtils.randomBytes,
        },
      },
      logger: logger.getChild('QUICClient'),
    });
  } catch (e) {
    console.log('FAILED TO CREATE QUIC CLIENT', e.name);
    throw e;
  }

  const stream = quicClient.connection.newStream();
  const reader = stream.readable.getReader();
  const writer = stream.writable.getWriter();

  // Remember to test out of order here
  await reader.cancel();

  // Write some bytes!
  for (let i = 0; i < 100; i++) {
    await writer.write(data1KiB);
  }
  await writer.close();

  await testsUtils.sleep(100);

  // This is forcing the connection stop to occur, which is code 0
  // It waits however for the stream to gracefully close, which occurred
  // You still get the connection error early though without delays
  // If there is a delay, that will ensure that close is in fact sent out
  // Processed... and therefore, destroy here works fine too
  await quicClient.destroy({ force: false });

  await quicServer.stop({ force: false });

  // This would mean... that we don't force it... we wait gracefully for the streams to just close
  // And it should still work
  // So graceful closing works
  // await quicClient.destroy({ force: false });

  // What about non-graceful close?
  // What if I force destruction of the QUIC client?
  // Then what?
  // In this case, it won't wait for the streams to be closed gracefully
  // It cancels and aborts the streams
  // And then proceeds to immediately send connection close frame but allows the server to clean up

  // await quicClient.destroy({ force: true, applicationError: false, errorCode: 1 });

  // Ok this works too, and it's because the stream was in fact already closed earlier

  // Now what if we don't wait for the client to destroy?
  // await testsUtils.sleep(1000);
  // In that case client is destroying force
  // Sending out and timing out
  // And the server is process and graceful close
  // It should still work too

  // If the connections are all gone, we shouldn't need to do this
  // await quicServer.stop({ force: false });

  // What if we now try to force it here too?
  // await quicServer.stop({ force: true });
  // It works

  // let's see what happens if we shutdown both at the same time
  // await Promise.all([
  //   quicClient.destroy({ force: true, applicationError: false, errorCode: 1 }),
  //   quicServer.stop({ force: true }),
  // ]);
  // It works

  // What about gracefully stopping both concurrently?
  // await Promise.all([
  //   quicClient.destroy({ force: false }),
  //   quicServer.stop({ force: false }),
  // ]);
  // It works

  // Let's try not closing the streams properly
  // await Promise.all([
  //   quicClient.destroy({ force: true }),
  //   quicServer.stop({ force: true }),
  // ]);
  // That works too, so that will force close stuff

  // What about if graceful close, without closing any of the streams
  // await Promise.all([
  //   quicClient.destroy({ force: false }),
  //   quicServer.stop({ force: false }),
  // ]);

  //oh boy it is stuck!!
  // It is precisely stuck on quic client destruction
  // because the quic client stream exists
  // but the server does not... funnily enough
  // so the server's connection is immediately started and stopped
  // but the quic stream is still not fully resolved
  // which makes sense here

}

void main();

Final checklist

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

[ghost]

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

Legend

[CMCDragonkai]

If the QUICSocket is encapsulated in the QUICServer. Then stopping the QUICServer should stop the QUICSocket. And this will result in an emission of EventQUICSocketStop.

If QUICSocket is injected into QUICServer. Then stopping the QUICServer should not stop the QUICSocket. And this will not result in an emission of EventQUICSocketStop.

When EventQUICSocketStop is emitted. It has to be listened by QUICServer whether it is encapsulated or injected. This event could emit at any time due to any problem or user-initiated call. Therefore we must proceed to then stop the QUICServer.

One issue is that when stopping the QUICServer, we are also stopping the QUICSocket. Which results in an emission of EventQUICSocketStop. If we handle this to stop the QUICServer, don't we have an infinite loop?

Well atm, it's a no, because due to StartStop and associated decorators, we are using locks to block and furthermore conditions to coalesce these calls. So the infinite loop is terminated by the async-init decorators.

However as a general pattern, we shouldn't depend on the async-init in this situation. There could be other situations where an event is emitted from another object that we need to handle, which causes to execute a pull-method which triggers another event of the same type, thus creating an infinite loop.

Therefore, rather than simply relying on async-init to cancel the infiniteness (not only is this relying on implicit behaviour, but it's not even guaranteed in the future). We need something else as a general pattern to prevent such things.

Here are some possibilities.

1. We stop listening for the EventQUICSocketStop when we perform the quicSocket.stop(). - This is rather error prone, as we it requires us to keep track of this context every time we call quicSocket.stop(). And I err against the side of things that require too much context to keep track of.
2. Could the handler itself terminate with a base case? How would it decide? It cannot decide based on whether QUICSocket is injected or encapsulated, because in either case the EventQUICSocketStop could be emitted. Perhaps instead it can rely on a certain state-variable representing the execution of QUICServer.stop?
3. Other solutions?

Note that solution 2 means rather than just calling void this.stop(), it would rely on checking if the status of the system is stopping. Like if (this[stopping]) { ... }.

Is this actually any better than solution 1? Or just relying on void this.stop since the decorator manages it.

I think it is better than solution 1 as it centralises logic.

It's better than simply relying on this.stop from the decorator because it is explicit and gives a model for future operations.

The presence of infinite loops here is primarily due to this 1 reason: the inability to easily distinguish if an event happens due to something I'm already doing. The only good solution is to rely on status conditionals representing the state of doing that operation.

[tegefaulkes]

I think it ties into the idempotent nature of the stop method. In this case it's handled by the decorator. but as a general rule, for things like this, it can only really stopped or destroyed once. So the stop or destroy event can only be emitted once.

In the case of stop, it's emitted only once per state change since it could be started again. In that case it still prevents the cycle.

This might generalise as, any event that is the result of a state change, can only emit once per state change? we can refer to them as state events or... Once-lers? In any case, how does that sound for a general rule?

[CMCDragonkai]

Now on the topic of encapsulated events. Where QUICServer encapsulates QUICSocket. Another design decision is about how encapsulated events should be emitted out of QUICServer.

Firstly there will be "relevant" and "irrelevant" events to QUICServer.

The "relevant" events are events that QUICServer must react to.

The "irrelevant" events are events that QUICSocket does not need to react to.

In both cases, there's a question about whether such events should be bubbled upwards outside of QUICServer.

1. Does an event matter to the outside world? Is QUICSocket an entirely encapsulated system?
2. If we do bubble it up, how do we do this? Currently Event is not allowed to be re-dispatched. We can only either dispatch a new event that wraps the original event by setting the original event as the detail property of the new event, or we redispatch the original event by instantiating a new class of that same event, and copy any detail to this new instance.

In terms of 1. even if we could argue that certain events shouldn't be relevant. Dispatching such an event is still useful for "observability". Where it can be useful to debug the internal state of the system, such as for tracing. The question is whether such tracing should make use of these events, or if a separate system should be used for such a thing. It also important to note that events could represent "pre-" or "post-" op events. However we defaulted to post- op events to match the convention of Node's event emitters and most other JavaScript evented systems.

In terms of 2., dispatching an event chain can help maintain a trace of these concurrent operations, similar to maintaining a trace of exceptions that was thrown, caught and rethrown. Exceptions are a bit easier since you can rethrow the same exception. I think that there are 2 equivalences:

• Rethrowing an exception is equivalent to re-dispatching the same event class with the same details. (Since you cannot redispatch the exact same instance).
• Throwing up a new exception chaining the old exception is equivalent to dispatching a new event wrapping around the old event.

Based on this https://gist.github.com/CMCDragonkai/08266b1463158f4156f66d4bf077add6 for exceptions, I think the same guidelines apply here.

# EXCEPTION RERAISE/RETHROW
# this reraises the same exception, in this case
# it just means you couldn't handle the exception
try:
    raise Exception
except Exception as e:
    raise e
    # raise # this is equivalent and you won't need to bind the exception
    
# EXCEPTION HANDLING EXCEPTION
# this raises another exception in the handling of the first exception
# python will recognise this and report it as so
# only do this if you really have an exception while handling an exception
try:
    raise Exception
except Exception as e:
    raise Exception

# EXCEPTION CHAIN
# this raises a new exception and chains the new exception with the old exception
# use this when your code represents an onion of different layers each capable of handling different exceptions
try:
    raise Exception
except Exception as e:
    raise Exception from e

I think both would be applicable depending on the situation.

In the case of QUICSocket and QUICServer, I think due to informational tracing, it would make more sense to just re-dispatch with the same class. This would be most of the events that we would want to redispatch. Then only in certain cases would we want to chain it for some reason.

[CMCDragonkai]

I think it ties into the idempotent nature of the stop method. In this case it's handled by the decorator. but as a general rule, for things like this, it can only really stopped or destroyed once. So the stop or destroy event can only be emitted once.

In the case of stop, it's emitted only once per state change since it could be started again. In that case it still prevents the cycle.

This might generalise as, any event that is the result of a state change, can only emit once per state change? we can refer to them as state events or... Once-lers? In any case, how does that sound for a general rule?

I think using once: true would actually in fact make sense here if the event handler could be specialised to that specific event that could trigger an infinite loop.

In situations where you have a common event handler, then once won't work because there could be other events you want to listen and handle all the time repeatedly.

[CMCDragonkai]

So it's possible to do this simultaneously:

1. Listen for "relevant" events, and handle it appropriately, as well as use once for things to terminate any infinite loops.
2. Listen for all events, and then the expectation is that all the events are "irrelevant", and thus can just be re-dispatched with the same class. The same-class redispatch can be done... but one then has to guarantee the signature of instantiating events. To do this, we can provide a clone method as part of a top-level root Event class. Similar to js-errors we can do AbstractEvent and provide a clone method.

Doing this would then enable using once for any infinite-loop event handles while also automatically re-dispatching irrelevant and relevant events.

This means "relevant" event handlers should not re-dispatch their events, but should just handle them with respect to QUICServer context.

[tegefaulkes]

I'm using this as scaffolding for the event with clone method for now. It will be replaced with your implementation.

type OptDetail<T> = T extends undefined ? {} : { detail: T };

class TempBaseEvent<T = undefined> extends Event {
  public readonly detail: T
  constructor(type: string, protected options?: EventInit & OptDetail<T>) {
    super(type, options);
    // @ts-ignore: Detail should be undefined here if it doesn't exist
    this.detail = options?.detail;
  }

  public clone(): this {
    // @ts-ignore: cheeky self prototype reference
    return new this.constructor(this.options);
  };
}

[CMCDragonkai]

As per MatrixAI/js-events#2, it should be easy to find and replace all uses to just AbstractEvent too. And due to the automation inside, most of the time you can just do:

class EventX extends AbstractEvent<SomeType> {}

And that's enough to create the relevant event.

When performing a redispatch you will need to do this.dispatchEvent(eventX.clone()).

Node 20 is now LTS so it should work...

[CMCDragonkai]

Some notes on redispatch:

Injection

Don't redispatch relevant and irrelevant events. Only handle relevant events.

Encapsulation

Redispatch all events including relevant and irrelevant events

On Redispatch

1. redispatch same event - only do this for "irrelevant" events
   • an info event on quic socket, is not an info event of quic server
2. redispatch chain event - only do this for "relevant" events
   • an error in quic socket, is an error of the quic server

Do 2. whenever there's an "equivalent" event on the domain model.

Do 1. when there is not.

When doing 2, the chain event structure should be always have a base type.

Consider this:

class EventQUICServerError extends EventQUICServer<Error | EventQUICSocketError> {}

Notice that I'm wrapping Error | ErrorQUICSocketError under EventQUICServerError.

• QUICServer might be encapsulating QUICSocket.
• If QUICServer encapsulates QUICSocket, any error event of QUICSocket is a relevant event to QUICServer. It gets handled, and the handling means QUICServer must destroy itself.
• This is a potential infinite loop, so we use once to terminate the loop.
• QUICServer redispatches QUICSocket using QUICServerError wrapping QUICSocketError because it's a relevant event and because a quic socket error is a quic server error.

This is not true for QUICConnectionError. While it is relevant to QUICServer for connection management, a quic connection error is not a quic server error. The quic server does not crash if there's a quic connection error. Thus redispatching upwards is purely informational. In this case, there is no need to wrap the event, and thus we redispatch the clone of it.

@tegefaulkes

[CMCDragonkai]

Let's see how the above structure plays out in code before reviewing.

[CMCDragonkai]

So one thing I've had to do is to introduce EventAll and EventDefault.

Furthermore my attempt at specialising the name property of AbstractError, EventAll and EventDefault fails to work because tsc won't accept the code even though it is now legal with native classes.

class Abc {
  // @ts-ignore
  public static name = `${this.name}`;
}

The above does not compile unfortunately. Even though it would be nice since we can continue using the same pattern like addEventListener(EventAll.name, f).

I used one before called EventAll.type, but then it would be different from the regular name property that would be used if people defined their own events.

addEventListener(EventCustom.name, f);
addEventListener(EventDefault.type, f);

[CMCDragonkai]

Ok the change of nameclashes is quite low. So I'm just going to use EventAll.name and EventDefault.name.

[CMCDragonkai]

The js-events MatrixAI/js-events#5 has made this comment possible:

#53 (comment)

2 examples of this is actually in the js-events tests.

Can follow that as a guideline on what to do @amydevs @tegefaulkes.

[CMCDragonkai]

Some notes on redispatch:

Injection

Don't redispatch relevant and irrelevant events. Only handle relevant events.

Encapsulation

Redispatch all events including relevant and irrelevant events

On Redispatch

1. redispatch same event - only do this for "irrelevant" events

   • an info event on quic socket, is not an info event of quic server

2. redispatch chain event - only do this for "relevant" events

   • an error in quic socket, is an error of the quic server

Do 2. whenever there's an "equivalent" event on the domain model.

Do 1. when there is not.

When doing 2, the chain event structure should be always have a base type.

Consider this:

class EventQUICServerError extends EventQUICServer<Error | EventQUICSocketError> {}

Notice that I'm wrapping Error | ErrorQUICSocketError under EventQUICServerError.

• QUICServer might be encapsulating QUICSocket.
• If QUICServer encapsulates QUICSocket, any error event of QUICSocket is a relevant event to QUICServer. It gets handled, and the handling means QUICServer must destroy itself.
• This is a potential infinite loop, so we use once to terminate the loop.
• QUICServer redispatches QUICSocket using QUICServerError wrapping QUICSocketError because it's a relevant event and because a quic socket error is a quic server error.

This is not true for QUICConnectionError. While it is relevant to QUICServer for connection management, a quic connection error is not a quic server error. The quic server does not crash if there's a quic connection error. Thus redispatching upwards is purely informational. In this case, there is no need to wrap the event, and thus we redispatch the clone of it.

@tegefaulkes

So this has changed a bit. On injection, we may redispatch the relevant event, if the handling of the relevant event, is itself an event on the object receiving the event.

[CMCDragonkai]

Rules for using js-events revised.

1. Dispatch event only after all state transition relevant to the event is done. This means events represent post-transition. This means subscribers always react to a post-transition state.
2. When an error event occurs in any given object X and is received by an outer object Y. Then it is Y responsibility to manage the lifecycle of X. But only if X is encapsulated within Y. Afterwards, Y will redispatch the X error event (whether encapsulated or not).
   
3. When handling events, if it is possible to hit an exception on the handler (both synchronous and asynchronous), it must be caught and redispatched as an error. Here's an example (notice the usage of AggregateError here, but when using js-errors, set the cause to be the AggregateError). Example:

      protected handleEventXError = async (evt: EventXError) => {
        try {
          // Transition Y to errored state...
          if (this.isXEncapsulated) {
            // Only if it is encapsulated.
            await this.x.stop();
          }
          this.dispatchEvent(
            new EventYError({
              detail: evt,
            }),
          );
        } catch (err) {
          // Unexpected error is also dispatched
          this.dispatchEvent(
            new EventYError({
              // Aggregate error because it's actually 2 errors
              // If you use `js-errors`, then try making the cause an array
              // The array order should be
              detail: new AggregateError([err, evt.detail]),
            }),
          );
        }
      };
   

4. Say X emits error, then Y handles and tries to stop X then redispatches, then Z handles and tries to stop Y... etc all the way up to the root of the program P. It is important that at P, event handlers are added, otherwise errors will disappear to the ether.
5. Say X emits error, then Y handles and tries to stop X, but it fails at this point with a thrown exception. Then Y dispatches a new aggregate error. Z will handle this and try to stop Y. It may fail to stop Y too, in which case the same process repeats upwards.
6. Objects never listen on their own events.
7. Upon reacting to an event, mutations should occur before any event emission.
   
8. Event dispatch is always synchronous.
9. Event handlers which may cause errors should wrap the entire thing in a try catch that dispatches their domain error as it would be considered unexpected. That would be equivalent to calling await this.stop() so one can catch the exception.
10. The default handler should be used to redispatch the same instance of all encapsulated events.
11. Use EventDefault.name as a catch fall-through. See examples in js-events. Also use once for any event listener that may cause an infinite loop.
12. Always deallocate event listeners in opposite order.

In pull, handlers don't know who called them. In push, handlers always know who called them.

The usage of the event behaviour is tricky and not type-safe. Migrating to observables later in the future would enable these patterns in a more type-safe manner and allow us to automate this with higher order functions. But for libraries we have to stick with EventTarget.

[CMCDragonkai]

@tegefaulkes brought up a situation like this:

If an object has a pull-method that could result in an error, and the object itself is evented, does that mean:

1. Throw up the exception, but don't dispatch.
2. Dispatch an event error, but don't throw up the exception.
3. Dispatch an event error AND throw up the exception.

I think all 3 are possibly valid depending on the situation.

For 1., this makes sense if the pull-method doesn't actually do any state changes on the object. Thus the exception is only relevant to the caller.

For 2., this makes sense if you can say that the method is entirely internal to the library or module context. Like for example QUICConnection.recv.

For 3., this makes sense if the method performs a state change, and it's not an "internal" method. It is expected that things outside your library may call it. In this case, you have to document that it will dispatch and throw an exception. In which case, the user of the object has to decide what to do, whether it ignores one and handles the other.

[CMCDragonkai]

So for QUICConnection.stop it would need to do both dispatch and throw it upwards.

Note that in QUICConnection.recv which needs to do void this.stop().catch(() => {}) is because stop may throw up an error, and it needs to ignore it, because the expectation is that the error would have been already dispatched by QUICConnection.stop.

[tegefaulkes]

As it's currently implemented, the this.stop() is triggered in 4 conditions.

1. In recv() in response to a draining or closed state.
2. In send() in response to an error sending data through the socket.
3. In send() if the state transitioned to closed after sending data.
4. In the timer system in response to the connection timing out.

In cases 1, 3 and 4 it makes sense since we're responding to the the underlying quiche connection's state change.

2 is technically an internal error separate from quiche but it's a transport failure. This is the only one I can see emitting an error event that would trigger stop externally. I really don't like this since it's clean up logic critical to the function of QUICConnection that would need to handled external to the QUICConnection code and Maintained in 2 separate places in QUICServer and QUICClient. It makes things less maintainable.

Also, most errors coming out of quiche are obtained with connection.localError() or connection.peerError() after the connection has drained and closed.

[CMCDragonkai]

@tegefaulkes you will need to update the pkgs.nix, shell.nix to get node 20. Also there's a rule that you'll need to make use of arguments in .eslintrc. https://github.com/MatrixAI/js-events/blob/91a71b7c7ac04b8a29978533971dd0824da71af0/.eslintrc#L42

[CMCDragonkai]

As it's currently implemented, the this.stop() is triggered in 4 conditions.

1. In recv() in response to a draining or closed state.
2. In send() in response to an error sending data through the socket.
3. In send() if the state transitioned to closed after sending data.
4. In the timer system in response to the connection timing out.

In cases 1, 3 and 4 it makes sense since we're responding to the the underlying quiche connection's state change.

2 is technically an internal error separate from quiche but it's a transport failure. This is the only one I can see emitting an error event that would trigger stop externally. I really don't like this since it's clean up logic critical to the function of QUICConnection that would need to handled external to the QUICConnection code and Maintained in 2 separate places in QUICServer and QUICClient. It makes things less maintainable.

Also, most errors coming out of quiche are obtained with connection.localError() or connection.peerError() after the connection has drained and closed.

In your 2, you would follow 2.

[CMCDragonkai]

Also, most errors coming out of quiche are obtained with connection.localError() or connection.peerError() after the connection has drained and closed.

In this case, these errors are not errors of the QUICConnection.stop itself. They should just be dispatched.

[tegefaulkes]

So based on discussion,

The QUICSocket.send will not throw, but if it fails it will dispatch an error event. This event should ultimately cause a QUICConnection.stop() call. If it is managed by the QUICClient or QUICServer then handling is set up there based on encapsulation. It the QUICSocket is shared then the handling needs to be setup by whatever manages it.

Switching to this new pattern also means the QUICConnection shouldn't trigger a stop() call internally unless it drained and closed without error. If the quicheConnection enters the draining or closed state then we need to check for an error. Dispatch said error, then whatever is managing the connection, QUICServer or QUIClient, MUST call stop() on that connection while passing the relevant information back into it.

The QUICServer needs to trigger QUICConnection.stop() if it handles an error. If the QUICSocket is encapsulated then any socket errors needs to be handled and trigger stopping of the socket. Then re-dispatched so whatever manages the QUICServer can clean up the QUICServer. It the QUICSocket is not encapsulated then what ever manages both the QUICSocket and QUICServer needs to handle their errors and clean them up.

Likewise for the QUICClient.

[CMCDragonkai]

When separating the functionality between encapsulated dependencies and injected dependencies, we have to basically have a boolean that branches out the functionality.

However this results in alot of boilerplate. Requiring a separate xIsInjected boolean for every dependency that may be encapsulated or injected.

Only a few dependencies make use of this distinction during runtime. Most of the time, this is actually only used for testing & mocking.

Consider for example in the 3 situations of EFS, QUIC. In EFS, the db can be encapsulated or injected. In QUIC, the QUICSocket can be encapsulated or injected. Both cases are legitimate usecases. We should definitely have a boolean that indicates the distinction.

Consider the case of PolykeyAgent. Almost all of its dependencies should just be fully encapsulated. The only reason to enable the ability to pass in DB,... etc is allow mocking or unit testing. But the amount of boilerplate to support all of the dependencies would be HUGE!

The only way to make such a thing manageable would be to introduce parameter decorators on the constructor that automatically produces properties.

However I don't want to do that right now, it might introduce too much complexity. Instead let's do something different.

1. Where we have legitimate reasons for encapsulation/injection, we continue doing it with a manually specificed xIsInjected or equivalent boolean.
2. Where it's only there for testing, we can remove it..., and instead rely on module mocking in jest to make use of it. See: https://jestjs.io/docs/mock-functions#mocking-modules

This doesn't have to be a wholesale change immediately... we can just keep an issue in PK and slowly apply this across the ecosystem. In some cases, optional injection can still be kept since there's barely any lifecycle related activities, or the relevant objects in question are not evented objects.

---

MatrixAI/Polykey#558

[CMCDragonkai]

If dispatching EventQUICServerError, means that QUICServer is in an errored state, does that mean we need a proper state transition like this.error().

Perhaps instead we would say this.stop({ error }) as indicative of transitioning to a stopped-error state.

[CMCDragonkai]

I've reworked all the commits, the last commit still wip. But this MR is ready. We can merge now.

Also confirmed that a lazy timer indeed only changes the status lazily. So it does need to be deleted. Only non-lazy timer will synchronously change the status.