Add support for the new WebSocket notification protocol #1468

joachimvh · 2022-10-03T12:51:45Z

There is going to be a lot of text in this comment as there is a lot to cover.

Adds support for the notification protocol. The editor's draft was also used as inspiration for certain parts. Specifically WebSocket subscriptions were added as that one has a published spec, but after this PR I might also look into adding webhooks due to the demand stated in the other PR.

Also adds basic support for a storage description resource as defined in the editor's draft of the 0.9.1 solid protocol spec as that is used for discovery of notification channels.

Alternative to #1388 as that one is based on an older version of the spec as discussed there.

As can be seen in the commits, there are first several utility commits before getting to the notification part.

Architecture

Markdown documentation was added explaining the architecture but will also summarize here.

There are 3 parts relevant for notifications:

Exposing how to subscribe to notification channels
Handling subscriptions on channels
Emitting events to subscriptions on activity

I will cover the solutions for these separately below.

The main focus of the architecture was making sure the work for adding a new notification type is minimal and can reuse most of the existing components, but on the other hand still allows enough flexibility to support differences between the types. I feel like I mostly succeeded but there are still some things that feel like improvement might be possible so input is always appreciated.

Discovery

The editor's draft of the notification spec recommends using either the storage description resources for discovery, or the description resource of the resource you want to subscribe to. Seeing as how we handle description resources is still changing a lot, and we have to add a storage description resource for v0.9.1 in this PR. The latter could still be added somewhere in the future.

There are actually 2 discovery steps here: first one has to discover the storage description resource, and then that one has to be used to discover the notification channels.

Discovering the storage description

At the time of writing, the editor's draft of v0.9.1 states:

Servers MUST include the Link header with rel="http://www.w3.org/ns/solid/terms#storageDescription" targeting the URI of the storage description resource in the response of HTTP GET, HEAD and OPTIONS requests targeting a resource in a storage.

To this end, the StorageDescriptionAdvertiser (a MetadataWriter) was created. For every request it will recursively look for parent containers until it finds one that is a pim:Storage. The identifier of that container is then used to generate the above header. The identifier is appended with a fixed suffix we use for the storage description. Currently this is .well-known/solid. So if my pod is http://localhost:3000/joachimvh/, a request to http://localhost:3000/joachimvh/card/profile#me will have the storage description response header with value http://localhost:3000/joachimvh/.well-known/solid.

Initially I thought about just using http://localhost:3000/joachimvh/.meta for the storage description resource, but that resource is not always accessible (due to read permissions) so can't be used for that.

Generating the storage description

Due to the fixed suffix used for storage descriptions we can catch all requests targeting such a resource in a single handler. There we just have to make sure that the container, after removing the suffix, is a storage container.

To generate the output, there is the StorageDescriber interface which returns an array of quads. These then get merged and returned as output using content negotiation. This way we can combine multiple components that each want to put something in the description. Currently this is the required fixed triple (stating that the container is a storage), and the descriptions of the notification channels.

Due to using quads, we currently don't guarantee that the JSON-LD output for notification discovery will have the expected format. But since there are still ongoing discussions about that, this seemed like a good solution for now. But it might still change in the future.

Specifically the NotificationDescriber is a StorageDescriber that generates the necessary triples for a notification channel. It does so by having a fixed URL for every type. E.g., for websockets we have /.notifications/WebSocketSubscription2021. This is the URL that clients need to discover to be able to subscribe.

Subscribing

To handle subscriptions there is the NotificationSubscriber class. Should be wrapped in a router handler and a parsing handler so it receives Operations targeting a specific notification URL (such as the one mentioned above). It takes as input a SubscriptionType, which is class that contains all the relevant information for a specific type. SubscriptionType is one of those interfaces that feels like it could be improved to have a cleaner architecture.

First the SubscriptionType is used to parse the input JSON body. Here the yup library is used to have a base parser that can be extended if needed.

After that the SubscriptionType returns which permissions are needed to fulfill this specific subscription. These are then used to run the standard authorization process.

Finally the SubscriptionType is called to do whatever is required for a subscription of this type. Generally this will involve storing the subscription in a SubscriptionStorage for later reuse. It also returns the response to return to the client.

The SubscriptionStorage is the reason I created #1450.

Emitting notifications

The ListeningActivityHandler is the new class that listens to the events emitted by the MonitoringStore. It collects all relevant subscriptions from the storage mentioned above and for each of them calls it NotificationHandler with the necessary information. This handler is responsible for making sure this info results in a notification being emitted.

In practice we have a ComposedNotificationHandler that splits up the process into 3 separate steps and takes a separate handler for each of those steps:

A NotificationGenerator converts the input info into a Notification object.
A NotificationSerializer converts that object into a Representation.
A NotificationEmitter takes the Representation and somehow emits it as required.

One main reason for splitting up like this is reuse while allowing maximum flexibility. Several subscription types might all need to generate the same notification object, in a JSON-LD representation, without needing to add custom information. These would then all be able to reuse the same generator and serializer. On the other hand, there might be a subscription type that needs to add custom metadata to the representation before it can be emitted. In that case it is still possible to create a new serializer without having to change everything.

Another reason for this split is the accept feature from the notification spec. Clients can request to have their notifications sent to them in a different format from JSON-LD. This architecture allows us to have a reusable ConvertingNotificationSerializer in there which converts the resulting representation as necessary.

Having a separate generator also allows us to cache the generated notification which is useful since a backend call is required to create it.

State

Another feature from the notification spec is the state feature. If a subscription has this field, the server should emit a notification to that client in case the resource has a different state than the one specified. One problem is that how to do this differs greatly between different subscription types (and caused a lot of headache to find a decent solution). In the case of WebSockets for example, we can't send this notification until the WebSocket is connected. In the case of WebHooks on the other hand, this could be sent immediately, but probably should be sent after we returned the subscription response. There is a BaseStateHandler that can be used, but it is fully up to the implementations how and when this gets called. In the case of WebSockets the trigger is getting a matching connection.

The comment was made offline that we could just always send out a notification and just buffer it if necessary, but there are some issues with that. For one, we don't know what the notification should look like until the WebSocket is connected: the state of the resource could change between the subscription and the connection. Also if we start buffering notifications a client might receive multiple at once when they connect.

External/internal components

I would have 1 version of the server that supports both the legacy as the new notification type. After that we can look into removing the legacy solution. There is also something to be said for having the new notification solution in a separate repository. A lot of changes are still happening there and it might need faster update cycles. On the other hand, it might cause confusion for users to not have everything in 1 place, and with differing version numbers. Something to discuss in a separate issue afterwards perhaps.

Permissions

Although the editor's draft of the notification spec doesn't have specifics anymore, the implementation here requires credentials to have Read permissions on a resource. One potential issue that I see is that if the permissions on a resource change, the subscription still stays valid. Having to keep track of permission changes would increase the complexity of this solution though. It is possible to configure the server so all subscriptions have a maximum expiration time, which is one way to make sure they at least don't stay permanent.

Multithreading

Due to storing the WebSockets in memory, this solution only works on singlethreaded servers. A consequence is that all the default configurations we expose will now throw an error if a user tries to start it with multiple worker threads.

csarven · 2022-10-03T14:07:47Z

Generally this will involve storing the subscription in a SubscriptionStorage for later reuse.

+1. Unless already planned/possible, having SubscriptionStorage available through HTTP will support some use cases.

When a record of a subscription has its own URI - all information about the subscription agreement can be verified. It is also the identifier that can be used to refer to a subscription or a request to change the state of a subscription. We'll need to introduce a note or a requirement on that matter.

solid/notifications#111 as part of the idea to describe subscription requests and responses.

We're also trying to figure out the compatibility (or to what degree) between #subscription-resource (the target of the #subscription-request) and Solid container: solid/notifications#36 .

joachimvh · 2022-10-04T07:26:36Z

+1. Unless already planned/possible, having SubscriptionStorage available through HTTP will support some use cases.

The current implementation could support this but there are several practical considerations to be resolved:

Where would these resources be located? Would this be somewhere in the same storage as the one where the resource is located or would we create a separate storage specifically for subscriptions?
How would access control work on these new types of resources? A new ACL/ACR resource would have to be created for every subscription to control this. Only the WebID that created the subscription should also have access to it, because otherwise someone else could use it to also open a WebSocket for example.
There should be several restrictions on how clients are allowed to edit this resource (or not have any write permissions at all), since there are several values in there that shouldn't be changed.
Every time a resource changes we need to know which are the associated subscriptions. So we still need to keep a mapping somewhere that maps resources to subscriptions. And this mapping needs to be kept in sync with the state of the subscriptions (e.g., if a subscription gets deleted).
(Probably some other things I'm not thinking of right now).

ixuz · 2022-10-04T13:12:13Z

I would have 1 version of the server that supports both the legacy as the new notification type. After that we can look into removing the legacy solution

By "legacy solution", do you mean the implementation done in #1388? If that's the case, it would definitely make transition from legacy to new solution smoother. :)

joachimvh · 2022-10-04T13:23:56Z

By "legacy solution", do you mean the implementation done in #1388?

No, I meant the previous WebSocket notification solution that existed before this. Merging the result of #1388 with this is not feasible due to the differences. The plan is to look into supporting WebHooks with the architecture here after it is merged.

ixuz · 2022-10-04T13:25:17Z

By "legacy solution", do you mean the implementation done in #1388?

No, I meant the previous WebSocket notification solution that existed before this. Merging the result of #1388 with this is not feasible due to the differences. The plan is to look into supporting WebHooks with the architecture here after it is merged.

Alright understood! Sounds good! 👌

joachimvh · 2022-10-06T13:18:42Z

Did a small change so this also closes #440 since there was a handler in there that pretty much did what was asked there.

RubenVerborgh · 2022-11-03T12:42:06Z

Due to storing the WebSockets in memory, this solution only works on singlethreaded servers.

Is there an issue to address this (and do we have an idea of how to do it)?

joachimvh · 2022-11-03T12:58:35Z

Is there an issue to address this (and do we have an idea of how to do it)?

No. Makes more sense to create the issue if this is merged I think? But I don't see an easy solution for this. We would need a way to communicate across worker threads, similar to #726.

RubenVerborgh · 2022-11-03T13:00:23Z

Or completely decouple the WebSockets server, and have a (single) HTTP or socket line open from the main server to the WebSockets server.

joachimvh · 2022-11-03T13:05:47Z

Or completely decouple the WebSockets server, and have a (single) HTTP or socket line open from the main server to the WebSockets server.

True, that is also an interesting idea. Then that part is singlethreaded though.

RubenVerborgh

Reviewed first 2 commits so far. Going well; just the ServerListener thing was very confusing, but I proposed a solution.

src/storage/MonitoringStore.ts

src/server/notifications/ActivityEmitter.ts

RubenVerborgh · 2022-11-03T12:55:03Z

src/util/GenericEventEmitter.ts

+ * but with specific typings based on {@link GenericEventEmitter}.
+ * Useful in case a class needs to extend {@link EventEmitter} and wants specific internal typings.
+ */
+export function createGenericEventEmitterClass<T extends EventEmitter>(): (new() => T) {


I find it a bit weird with the eslint-disable-next-line @typescript-eslint/naming-convention; somehow I think this can be easier. Can't we just export a constant named EventEmitter<T> here, which is actually the typed version of the Node EventEmitter?

GenericEventEmitter<T> is the typed version of EventEmitter. The interface that is. The problem is classes that extend EventEmitter, such as MonitoringStore. if they still extend EventEmitter you would not have the correct typings internally. So this.emit(...) would not check for correct typings. Even if the class is defined as class MonitoringStore extends EventEmitter implements GenericEventEmitter<...>. The only way to have the internal typings be correct is to extend a class that has correct typings (or rewrite all the functions internally).

So what we need is a class that is an implementation of EventEmitter, but actually says its interface is GenericEventEmitter<T>. The above function generates such a class.

It might be possible to create a class with generics that can be extended immediately. All EventEmitter functions would have to be reimplemented though to cast them as TS would complain the output is not correct (although we know it is, which is why we can create this interface). But then that wouldn't allow us to combine multiple GenericEventEmitter interfaces like we do for the activity emitter.

One of the problems is that there are less casting opportunities when extending and there is a clear difference between classes/implementations and interfaces/types/descriptions.

... or there is a different solution I didn't think of yet 😄 .

export class EventEmitter<T> where the constructor returns new events.EventEmitter as any? But yeah, you'd still need to type out the empty methods I presume.
And I see the problem of combining things indeed.

config/example-https-file.json

src/server/HandlerServerListener.ts

RubenVerborgh · 2022-11-03T13:08:04Z

src/init/ServerInitializer.ts

    this.port = port;
  }

  public async handle(): Promise<void> {
    this.server = this.serverFactory.startServer(this.port);
+    await this.serverListener.handleSafe(this.server);


Mmm serverListener is really a misnomer here; it is the object that attaches a server listener.
I would have called it an initializer, but then ServerInitializer is already a thing.

However, it seems that ServerInitializer is not doing a lot now. Could we just merge ServerInitializer into the concrete server factory? But then we have the finalize indeed…

Maybe ServerInitializer becomes ServerManager or ServerRunner, and ServerListener becomes ServerInitializer, because it really only executes once.

That said, given that it only executes once… maybe the (current) ServerListener (renamed to ServerInitializer) should instead be a parameter to the server factory; it's their job to construct them in. Then the (current) ServerInitializer indeed becomes simply ServerRunner, whose job it is to start a server on a certain port of socket.

But ServerRunner still extends Initializer right? And ServerInitializer would not extend Initializer, so that might cause some confusion.

Ah right, Initializer is a thing too. Even though it's just a simple async handler. Mmm… So do we really need it on ServerRunner?
Or any other naming suggestions? ServerConfigurator or so for the current listeners?

So do we really need it on ServerRunner?

We need an Initializer that triggers everything related to creating a server and listening to the port, which is what ServerInitializer currently is. An Initializer is a handler that gets executed when the process starts so it's a fixed concept in CSS. I'm OK with ServerConfigurator. Most names would be fine to me, would just not reuse the term Initializer.

ServerConfigurator it is!

That said, given that it only executes once… maybe the (current) ServerListener (renamed to ServerInitializer) should instead be a parameter to the server factory; it's their job to construct them in.

Problem is that startServer is a sync function while the ServerConfigurators are handlers and thus async. Although I don't really see a problem with making startServer async as well.

test/integration/LegacyWebSocketsProtocol.test.ts

RubenVerborgh

Great work!

config/http/handler/handlers/storage-description.json

src/server/description/StaticStorageDescriber.ts

src/server/description/StorageDescriptionHandler.ts

src/util/handlers/CachedHandler.ts

src/server/notifications/Notification.ts

RubenVerborgh · 2022-11-03T16:24:20Z

src/server/notifications/WebSocketSubscription2021/WebSocket2021Emitter.ts

+  public async handle(input: WebSocket2021HandlerInput): Promise<void>;
+  public async handle(input: NotificationEmitterInput | WebSocket2021HandlerInput): Promise<void> {
+    if (this.isWebSocket2021HandlerInput(input)) {
+      // Called as a WebSocket2021Handler: store the WebSocket


Mmm this is a bit spooky. It's because we only have one handle method, right?
I think we should just break the handler pattern here then; and if we need it, have a small inner object for subscriptions?

Darn I was hoping you were going to be "this is super cool!". Though about making a utility handler for this even 😉 .

But yes, it's because of the single handle call. I initially had a solution with 3 classes, one of them being the small inner object that just stored the websockets (which is the thing both these handlers need). But it felt silly to make the configuration more complicated for these tiny classes.

I wonder if it's possible in TypeScript to have a generic class, where handle is part of the generic signature, such that the method names can be different 😅

But I really wouldn't overload handle like this. And if we really must (I don't think we do), we should dispatch it to 2 different internal methods then; not have the code inline.

src/server/notifications/WebSocketSubscription2021/WebSocket2021Listener.ts

This allows us to have typed EventEmitters.

This allows us to decouple the WebSocket listening from the HTTP configs, making these features completely orthogonal.

This allows us to route after an Operation has been parsed

There is a conflict between notifications being sent out right before a test stops, and the file locker trying to clean up the locks folder. This only has an impact when shutting down the server so has no real impact, but should still be fixed eventually so we can easily write tests.

joachimvh added the semver.major Requires a major version bump label Oct 3, 2022

joachimvh requested a review from RubenVerborgh October 3, 2022 12:51

joachimvh force-pushed the feat/notifications branch from 05f00d6 to 73a0b94 Compare October 3, 2022 13:02

joachimvh mentioned this pull request Oct 4, 2022

FileSystemResourceLocker seems to prevent the server process from stopping #1452

Closed

joachimvh force-pushed the feat/notifications branch from f11fb8a to edada43 Compare October 6, 2022 13:17

joachimvh force-pushed the feat/notifications branch from edada43 to 486e032 Compare October 6, 2022 14:26

RubenVerborgh linked an issue Oct 6, 2022 that may be closed by this pull request

Add CachedHandler #440

Closed

joachimvh force-pushed the feat/notifications branch from 486e032 to e1f8cd6 Compare October 7, 2022 06:10

joachimvh mentioned this pull request Oct 11, 2022

Extend and generalize InteractionRoutes #1478

Open

joachimvh force-pushed the feat/notifications branch from e1f8cd6 to 27f4611 Compare October 12, 2022 07:16

joachimvh mentioned this pull request Oct 13, 2022

Add WebHookSubscription2021 support #1481

Merged

RubenVerborgh reviewed Nov 3, 2022

View reviewed changes

joachimvh force-pushed the feat/notifications branch from 27f4611 to 01dc8c3 Compare November 3, 2022 13:33

RubenVerborgh approved these changes Nov 3, 2022

View reviewed changes

joachimvh added 6 commits November 7, 2022 14:47

feat: Create a GenericEventEmitter interface

afc94cb

This allows us to have typed EventEmitters.

feat: Split up server creation and request handling

b19d2b0

This allows us to decouple the WebSocket listening from the HTTP configs, making these features completely orthogonal.

feat: Create an ArrayUnionHandler which flattens the sources results

78c8709

feat: Create an OperationRouterHandler

3daf7b1

This allows us to route after an Operation has been parsed

feat: Expose a storage description resource for storage containers

994edd7

feat: Create a CachedHandler that caches AsyncHandler results

2736222

joachimvh added 4 commits November 8, 2022 11:45

feat: Add support for the Notification specification

c6529ca

feat: Add support for WebSocketSubscription2021

ee16b68

docs: Add notification architecture documentation

86b36aa

joachimvh force-pushed the feat/notifications branch from 01dc8c3 to 81c9d88 Compare November 8, 2022 14:56

test: Reduce integration test memory usage

19f2bb7

joachimvh merged commit 80fa81a into versions/6.0.0 Nov 9, 2022

joachimvh deleted the feat/notifications branch November 9, 2022 08:10

joachimvh mentioned this pull request Nov 9, 2022

Look into jest memory issues #1496

Closed

joachimvh mentioned this pull request Jul 18, 2023

Add CachedHandler #440

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for the new WebSocket notification protocol #1468

Add support for the new WebSocket notification protocol #1468

joachimvh commented Oct 3, 2022

csarven commented Oct 3, 2022 •

edited

joachimvh commented Oct 4, 2022

ixuz commented Oct 4, 2022

joachimvh commented Oct 4, 2022

ixuz commented Oct 4, 2022

joachimvh commented Oct 6, 2022

RubenVerborgh commented Nov 3, 2022

joachimvh commented Nov 3, 2022

RubenVerborgh commented Nov 3, 2022

joachimvh commented Nov 3, 2022

RubenVerborgh left a comment

RubenVerborgh Nov 3, 2022

joachimvh Nov 3, 2022

RubenVerborgh Nov 3, 2022 •

edited

RubenVerborgh Nov 3, 2022

joachimvh Nov 3, 2022

RubenVerborgh Nov 3, 2022

joachimvh Nov 3, 2022

RubenVerborgh Nov 3, 2022

joachimvh Nov 7, 2022

RubenVerborgh left a comment

RubenVerborgh Nov 3, 2022

joachimvh Nov 4, 2022

RubenVerborgh Nov 4, 2022

Add support for the new WebSocket notification protocol #1468

Add support for the new WebSocket notification protocol #1468

Conversation

joachimvh commented Oct 3, 2022

Architecture

Discovery

Discovering the storage description

Generating the storage description

Subscribing

Emitting notifications

State

External/internal components

Permissions

Multithreading

csarven commented Oct 3, 2022 • edited

joachimvh commented Oct 4, 2022

ixuz commented Oct 4, 2022

joachimvh commented Oct 4, 2022

ixuz commented Oct 4, 2022

joachimvh commented Oct 6, 2022

RubenVerborgh commented Nov 3, 2022

joachimvh commented Nov 3, 2022

RubenVerborgh commented Nov 3, 2022

joachimvh commented Nov 3, 2022

RubenVerborgh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RubenVerborgh Nov 3, 2022 • edited

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

RubenVerborgh 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

csarven commented Oct 3, 2022 •

edited

RubenVerborgh Nov 3, 2022 •

edited