StreamingHTTPChannel2023

[elf-pavlik]

📁 Related issues

closes #1574

✍️ Description

Initial commit only supports pre-established notification channels advertised in the HTTP Link header.
We will start writing tests after the initial feedback.

✅ PR check list

Before this pull request can be merged, a core maintainer will check whether

• this PR is labeled with the correct semver label
  • semver.patch: Backwards compatible bug fixes.
  • semver.minor: Backwards compatible feature additions.
  • semver.major: Breaking changes. This includes changing interfaces or configuration behaviour.
• the correct branch is targeted. Patch updates can target main, other changes should target the latest versions/* branch.
• the RELEASE_NOTES.md document in case of relevant feature or config changes.
• any relevant documentation was updated to reflect the changes in this PR.

[joachimvh]

Had a quick look. The idea seems fine, just requires some cleanup.

[elf-pavlik]

It doesn't resolve on node 21
I will focus on the other cleanup in case you have an idea of the problem. Otherwise, I will return to it as the last step in wrapping up this PR.

[joachimvh]

It's unfortunate that it only happens on Node 21. Might mean it is just the consequence of an issue in that version that could get fixed in the future. You might want to play around with disabling tests in the suite to see if it is one specific test that is the cause. I remember running into similar issues when implementation the other notification types because a connection wasn't closed causing the tests to not halt.

[elf-pavlik]

CommunitySolidServer/src/init/ServerInitializer.ts

Line 46 in f59b2c2

return promisify(this.server.close.bind(this.server))();

Adding this.server.closeAllConnections() before this line works, so it looks like, in node >21, some requests in the test are still awaiting a response.

[elf-pavlik]

When I start the server, change gets emitted for http://localhost:3000/.internal/setup/current-server-version, which leads to this line resulting in [GuardedStream] {Primary} error: No error listener was attached but error was thrown: premature close
I'm considering changing the StreamingHttpListeningActivityHandler not to emit notifications for changes on resources matching ./internal/

[joachimvh]

I would expect this error to then get thrown every time a resource is modified and streams is not defined, is that not the case?

What happens if you first attach an error listener to the data stream before destroying. Does it still give an error then? It's a safety mechanism in CSS to make sure there is always an error listener attached to a stream.

[elf-pavlik]

I would expect this error to then get thrown every time a resource is modified and streams is not defined, is that not the case?

You are right!

Since the WebSocket2023Emmiter does the same

CommunitySolidServer/src/server/notifications/WebSocketChannel2023/WebSocket2023Emitter.ts

Line 33 in f59b2c2

representation.data.destroy();

I ran a quick test, and no error was logged if I created a web socket channel without specifying accept.
On the other hand, the same error was logged if I created a web socket channel while specifying accept: text/turtle. I haven't looked into how representations get converted, but it seems like a stream without a listener might be created in that process.
I'm going to triple-check it, but if the same error gets logged for WebSocketChannel2023 with accept: text/turtle, can we track it as a separate issue and solve it outside of this PR?

---

I still want to double-check with you if it is intentional that the ResourceStore emits changes on resources inside .internal directory. If it is, do you see it reasonable if I filter using a regex to check the resource identifier and don't emit notifications for anything inside the .internal. I'm also wondering if I will need to pass the baseUrl to my StreamingHttpListeningActivityHandler or could use a regex without the baseUrl?

[joachimvh]

If I understand correctly, the difference is that with the WebSocket solution, that code will only be reached if someone registered a channel to listen to that resource. Otherwise no notification would be created and the server would not attempt to emit it.

The difference here, and this is related to the second question as well, is that StreamingHttpListeningActivityHandler will always generate a notification and always attempt to emit it, as it doesn't know if someone is listening or not, causing it to also generate notifications for the .internal container, and any other resource that gets modified but nobody is listening to. With the other notification solutions this gets cut off before the notification gets generated instead. As a solution, it might make sense to also give the StreamingHttpListeningActivityHandler access to the StreamingHttpMap so it can do an existence check before calling the NotificationHandler?

[TallTed]

Typo, grammar, punctuation...

[elf-pavlik]

When the client disconnects from the streaming response, the following gets logged.

[BasicResponseWriter] {Primary} error: Aborting streaming response because of server error; headers already sent.
[BasicResponseWriter] {Primary} error: Response error: premature close
[HandlerServerConfigurator] {Primary} error: Request error: aborted

I see it as normal behavior for the client to disconnect once they want to stop receiving notifications. It can also happen on a spotty connection. Should it be logged out as an error?

[joachimvh]

I see it as normal behavior for the client to disconnect once they want to stop receiving notifications. It can also happen on a spotty connection. Should it be logged out as an error?

You could attach an error listener to catch that one and emit a more detailed/correct error instead, but if this is only on disconnect this is not a big issue.