Improvements to SSE #5853

Arlen22 · 2021-07-06T01:16:15Z

The main change to the server is that it keeps the last 5 minutes of events, and writes them to the client immediately when it reconnects if the client includes a last-event-id header (which EventSource does automatically). Obviously we have to debounce that for tiddlyweb, but it's part of the spec and makes the server properly usable for other plugins. If it cannot find the last-event-id that the client specified it returns a 409 Conflict, which cause the EventSource to throw an error and close.

EventSource is the browser class that calls the server to recieve server-sent events.

The main change in the client is that whenever the EventSource throws an error, if the EventSource has closed, the client will now call syncFromServer to get the latest tiddlers when it successfully reopens the connection. The EventSource also throws an error when reconnecting automatically, but the readyState is set to CONNECTING so I just ignore it.

I also renamed the server parameter sse-enabled to tiddlyweb-sse-enabled since it is tiddlyweb specific and has no effect unless the tiddlyweb plugin is active. I renamed it in the status JSON as well.

I compiled the server-sent-events.js file from Typescript and it didn't make sense not to include the source code. I'm not sure what formatting changes are needed to the JS file. Obviously the eval code surrounds it with a self-executing function so doing that in the file isn't required. If you reallly don't want the typescript file feel free to not include it.

- Improved SSE server that properly handles event ids and saves the last 5 minutes of events for reconnecting clients. - Rename the server parameter `sse-enabled` to `tiddlyweb-sse-enabled` since it is tiddlyweb specific. - Properly handle reconnects in the client with the last-event-id header and manually refresh when connecting without last-event-id.

pmario · 2021-07-06T01:33:23Z

I also renamed the server parameter sse-enabled to tiddlyweb-sse-enabled since it is tiddlyweb specific and has no effect unless the tiddlyweb plugin is active. I renamed it in the status JSON as well.

IMO instead of renaming, the sse-enabled parameter we could check if all requirements are fulfilled and throw an error message or a warning if not. For me adding tiddlyweb to the name of the parameter doesn't indicate that it depends on a plugin. I expect it to work, if the parameter is set. ... If it doesn't work, it should tell me why.

Arlen22 · 2021-07-06T01:42:00Z

@Jermolene still working on this. Don't merge yet.

pmario · 2021-07-06T01:49:26Z

You can make the PR a draft so Jeremy will see it but can't merge.

- Move sse-client into the tiddlyweb adapter class itself and call when the status is received. - Generate an initial event id so reconnections work properly even if no events have been emitted yet. - Rename server variable in the status JSON

Arlen22 · 2021-07-06T03:02:04Z

I moved the sse-client.js file into the tiddlyweb adapter class. I don't know why I had it separate, but it works properly like this. It gets activated when the status JSON returns with tiddlyweb_sse_enabled.

I also fixed up the code that writes the events and made sure that every main case is getting all the correct stuff. I added an initial record entry so clients reconnecting after a server restart immediately get an event id.

There is also code in the Journal class for a second route (the emitter* functions) for clients to push events to the server to be broadcast, but this is not active in the tiddlyweb adapter. It is included however in case other plugins want to make use of that in some way. I did not test this much, but I did look over it and I expect it should work fine as it is very simple.

Arlen22 · 2021-07-06T03:06:09Z

I think the variable name should start with tiddlyweb because it is specific to that protocol, not to all sse in general. Other plugins will have to individually choose to honor that variable as it is checked in the tiddlyweb plugin, not in server-sent-events.js. The listen command already includes a check for the tiddlyweb plugin.

Arlen22 · 2021-07-06T03:19:13Z

Should I move the check to the server-sent-events Journal class into the core so it would completely disable all SSE instances? This would affect all plugins regardless of whether they are for syncing changes or running a chat app. If I did that it would make total sense to me to change the name back to sse-enabled, which is sort of why I'm asking, because right now it is only related to dynamicly loading changes through the tiddlyweb adaptor and so tiddlyweb-sse-enabled made the most sense to me.

- change name to repeater route - change repeater route to just return eventID

plugins/tiddlywiki/tiddlyweb/tiddlywebadaptor.js

pmario · 2021-07-06T04:55:48Z

plugins/tiddlywiki/tiddlyweb/sse-server.js


 \*/
-(function(){
+(function () {


there should be no spaces

TW core uses tabs to indent, for file size reasons.

plugins/tiddlywiki/tiddlyweb/sse-server.js

plugins/tiddlywiki/tiddlyweb/tiddlywebadaptor.js

plugins/tiddlywiki/tiddlyweb/sse-server.js

plugins/tiddlywiki/tiddlyweb/tiddlywebadaptor.js

Jermolene · 2021-07-06T10:09:34Z

Thanks @Arlen22. I'm sorry for any disappointment, but stepping back, I think these changes emphasise that the current SSE support is highly experimental and incomplete at this point. The trouble is that we're committed to backwards compatibility for anything that goes into the core, so there is a danger that we'll end up having to support features that we later come to regret.

At this point, I think the right thing to do is to completely revert all the current SSE commits for this release. What I'd like to do instead is add any necessary hooks so that these experiments can be done in plugins. I'd welcome other views @saqimtiaz @pmario

pmario · 2021-07-06T12:52:23Z

TLDR; I'm already relying on that feature. I would be OK with a plugin.

If it wouldn't be part of the next version at all, this would be a drastic setback for the German docs translation.

I didn't have a closer look at the code. I don't know if it's possible to use it as a plugin. @Arlen22 may know this.

long version

I did test the sse feature as implemented now and it worked well. I didn't have a look at the code, because I didn't need to.

I did setup a client server installation on my own server to be able to collaborate for the German translation of the TW documentation. See draft PR "German translation of the tiddlywiki documentation." #5855

The draft PR contains 490 changed tiddlers, that I got from a new contributor named Frank. I did get a single-file wiki and it was a complete pain to move all the tiddlers into new directories, so the content will be manageable. The "directory info" is completely missing in the single file wiki. ...

I want to use SSE to do collaborative editing, while we are connected via Skype. It is much more fun, if we change content like that and we can get immediate feedback.

SSE as it is, is not perfect, but for 2 users at the same time, it is good enough. I know, which tiddler I'm editing, because it's in my story river, and I can see the tiddler which he is editing, because it's the red one on the bottom of the screen. That's it.

pmario · 2021-07-06T12:55:10Z

ATM I'm preparing to test Arlen's code.

Jermolene · 2021-07-06T13:02:33Z

Hi @pmario thank you, more testing will be helpful.

The real-time update propagation is clearly very desirable functionality; the issue is whether this implementation is sustainable for the long term.

pmario · 2021-07-06T13:09:22Z

The real-time update propagation is clearly very desirable functionality; the issue is whether this implementation is sustainable for the long term.

I do understand your concerns and support them.

I think, if this functionality could be made a plugin, it would also be a good example for new plugins, that want to use the mechanism. ...

I think, SSE is much easier to handle as web-sockets and it still can be the first stage to something the community desperately needs. Collaborative editing for smaller teams.

core/modules/server/server-sent-events.js

pmario · 2021-07-06T15:05:04Z

@Arlen22 ... The problem I do have with your js-code is, that it contains a lot of unmaintainable code snippets, without the TS->JS transcoding stage. eg:

TypeScript

    initjournal(key: string) {
        if (!this.connections[key]) this.connections[key] = [];
        if (!this.records[key]) this.records[key] = [new JournalRecord("", "", 0, Date.now())];
        if (!this.entryIDs[key]) this.entryIDs[key] = 1;
    }

JavaScript

    Journal.prototype.initjournal = function (key) {
        if (!this.connections[key])
            this.connections[key] = [];
        if (!this.records[key])
            this.records[key] = [new JournalRecord("", "", 0, Date.now())];
        if (!this.entryIDs[key])
            this.entryIDs[key] = 1;
    };

As you can see it adds a linebreak between the if-clause and the if-body text, without any curly-braces. .. Since a 1-liner would be OK in JS too, a 2-liner is extremely error-prone when body content, or an additional linebreak is added in the future.

So from my point of view the JS-code is unmaintainable as it is at the moment.

- Fix braces in compiled JS by adding them in the source - Fix initJournal casing - Fix spacing

Arlen22 · 2021-07-06T19:43:24Z

I found an easy solution for the braces problem (just add them in the Typescript source). I also figured out where my auto formatter settings were and reconfigured them. Hopefully I got it all correct. Does this look any better?

Arlen22 · 2021-07-06T20:36:40Z

The server-sent-events.js file is a very important piece and I think that should be in core once we know it's stable, because it is just a generic SSE server that all plugins can create their own instance of and mount under their own path. The rest of it can all be in a separate plugin, in which case we would not need the server variable either, because the plugin is specified manually.

If we do add this line to the syncer.js file, that would make the rest of it very easy to do as a separate plugin. But we definitely should have the server-sent-events.js file in core at some point, because it's not plugin specific.

https://github.com/Jermolene/TiddlyWiki5/blob/23fec9e390115c2985e59d2ec1bf6d3cbf5e9a88/core/modules/syncer.js#L271

            $tw.hooks.invokeHook("th-syncadaptor-status-response", Array.prototype.slice.apply(arguments));

Not sure if that's the right name for the hook, but that pretty much describes it. It will fire whenever the status is recieved in the syncer, which is good because it is called several places, such as login and logout, when you need to reload the connection anyway. So I think it makes sense to fire any time the status is recieved.

- Set SyncDisablePolling to a status tiddler so it doesn't get synced. - Add hook to syncer.js - Remove server variable from status and docs. - Move the tiddlyweb related code into the plugin folder. - Nothing done to SSE server code

- Add warning to server side - Format sse-client.js with tabs - Remove check for server variable that I missed.

Arlen22 · 2021-07-07T03:22:50Z

Alright,

I moved the tiddlyweb related code into a plugin.
I added the hook to syncer.js right after the status tiddlers are set.
I renamed the SyncDisablePolling tiddler to be under $:/status instead of $:/config so it doesn't get saved to the server.
I removed the server variable since the plugin is specified manually for each datafolder. I also removed the docs.
I also removed it from the status and just set tiddlyweb to return false for isPollingDisabled. Other sync adaptors can return true if they like based on status or whatever, so I believe this is a really good idea which should be kept in.
The plugin will not set SyncPollingDisabled to yes until it successfully connects to the SSE endpoint. Reloading the page should revert it to default until it successfully connects again, because it is a status tiddler rather than config.
I left the server-sent-events.js file in place for now to demonstrate the separation of concerns as I see it. And also because I believe the feature set in that file is basically stable, and I haven't needed to change anything more with it.

Arlen22 · 2021-07-07T04:09:22Z

So basically it now looks like this. If someone wants SSE enabled for the TiddlyWeb sync adaptor, they enable the tiddlyweb-sse plugin. The plugin has all the setup code for both the server and client.

The server part consists of a route module which instantiates the Journal class, which handles all the business logic. Obviously many plugins can create instances of this class for their own purposes.

The client part consists of a startup module and hooks into the syncer status response. It then attemps to connect to the SSE endpoint and disables polling on the client if successful by setting SyncDisablePolling to "yes".

I change the hook to just pass in the Syncer instance and the status !err since everything is done on the wiki set on that instance, so anyone wanting to examine the syncer would really need to know which instance or at least which wiki the syncer was for. So syncer just made the most sense, in case there's more than one per wiki or who knows what else.

pmario · 2021-07-07T06:35:03Z

I did review the code. It looks good to me. I'll test the code a bit later today. ... I did find a problem yesterday, that was already there with the old code and should be addressed now. A bit more testing is needed

pmario · 2021-07-08T14:13:13Z

That line is the debouncer, which means there is likely a change happening every two minutes. Click on the wiki-change line in the network tab and click the event-stream tab in the detail. It will show you all the events that get emitted. It's possible a tiddler is being saved every two minutes.

There is no change happening! There are no details - it's empty. I can reproduce it with FF and Edge the same problem.
As long you do something with the wiki, you won't see it. You have to wait 2 minutes, doing nothing

pmario · 2021-07-08T14:15:07Z

If I double-click it in Edge it says

id: 162575343374311
retry: 5000

But it doesn't finish.

Edit: I saw, that this is intentional. So ignore the comment

pmario · 2021-07-08T14:19:49Z

The Edge console says:

:8080/events/plugins/tiddlywiki/tiddlyweb-sse/wiki-change:1 Failed to load resource: net::ERR_INCOMPLETE_CHUNKED_ENCODING

pmario · 2021-07-09T09:07:21Z

https://www.geeksforgeeks.org/node-js-http-server-timeout-property/ that's probably the problem. ... The default http module has a built in timeout of 2 minutes .. It seems?!

Obviously this isn't going into the PR, just putting it here for now while we work on it.

Arlen22 · 2021-07-09T15:56:02Z

Ok, not sure what that is, but I put a ten second setTimeout that closes the response to simulate the normal disconnection and reconnection and it does not do that for me. The chunked error is one that has something to do with the re-use of connections in HTTP/1.1. I ran into it the other day and I think it had something to do with making sure I immediately sent content or something like that.

Arlen22 · 2021-07-09T15:56:46Z

I moved all the files into a plugin for the moment, but did not edit any names. If you're using a datafolder to test this you do still need to put edit the tiddlywiki.info file, I think.

Arlen22 · 2021-07-09T15:58:25Z

There are some changes which do need to be made to core, including isPollingDisabled and the status hook, so I didn't revert those. I removed the server variable from the code and docs.

pmario · 2021-07-09T19:07:06Z

@Arlen22 ... I did dig it up. All of them. .. I'm using nodejs 12.x where the built-in timeout is 120 sec. See: https://nodejs.org/api/http.html#http_server_settimeout_msecs_callback -> open the History "Details" tag.

You can see, they changed this timeout to 0 in node 13. ... So if you use a later node version you don't see the problem.

So after 2 minutes the server closed the connection for me, and I saw the reconnection ... The problem I had is, that my "skinny tiddlers" already use 34kByte ... That I wanted to avoid. ... If server-cache is switched on, most of the times it was read from cache. So no real problem, but something I wanted to know, where it comes from.

pmario · 2021-07-09T19:14:32Z

net::ERR_INCOMPLETE_CHUNKED_ENCODING

Comes from the header that is used for SSEs. ... The SSE response header has Transfer-Encoding: chunked in the header, which is OK, since it is an open ended stream. It would need a slightly different encoding as we use. see: https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Transfer-Encoding

But I don't think we should change it. .. I actually don't care too much as I know where it comes from. ... I'll only try to fix it as an experiment, just to see if I did understand it right.

pmario · 2021-07-09T19:19:27Z

I did create a th-server-command-post-start hook, that checks the active timeout and sets it to 0 if it is 120seconds. ... So the timeout won't happen anymore. ..

There is only 1 new problem with https://nodejs.org/api/http.html#http_server_requesttimeout which must be set for node-version >14 ... if the server is directly connected to the web.

Arlen22 · 2021-07-09T19:59:37Z

Reconnections are part of the SSE protocol and should always be expected. If they are causing a refresh then that means something is wrong. If you can hover over the line in the list, not in the detail, it should tell you if there was an error.

Make sure you pull the latest changes just in case it's old code. But reconnections must be supported so I need to figure out why it's dong that. Can you give me anymore info about your setup? Are you running on localhost? And you have this happening in all browsers? They all run on the chrome engine now anyway, so probably not a big difference.

pmario · 2021-07-09T20:24:20Z

Reconnections are part of the SSE protocol and should always be expected. If they are causing a refresh then that means something is wrong. If you can hover over the line in the list, not in the detail, it should tell you if there was an error.

I know.

Make sure you pull the latest changes just in case it's old code. But reconnections must be supported so I need to figure out why it's dong that.

As I wrote, it was a problem from my nodejs version. I'm using 12.x ... Where there is a default timeout for http connections of 2 minutes. ... The client / browser does open a GET "wiki-change" connection, that shouldn't be closed ... But it was closed from the server. So the client initiated a reconnect. Starting from nodejs version 13 they did remove this 2minute timeout.

So if you use node > 13 you don't have this problem. .. I'm testing with FF and Edge

Can you give me anymore info about your setup?

node 12.x, FF latest, Edge latest windows 10

Are you running on localhost?

Yes port 8080 .. node .\tiddlywiki.js .\editions\de-AT-server\ --listen gzip=yes use-browser-cache=yes atm.

And you have this happening in all browsers?

Yes

They all run on the chrome engine now anyway, so probably not a big difference.

:) My main browser is FF. So I'm using a different engine most of the time. .. But I do like Edge. I think they do have a good new chromium based browser now.

pmario · 2021-07-12T18:35:41Z

@Arlen22 .. do you have a plugin repo already? Can you provide a link?

Arlen22 · 2021-07-14T14:08:52Z

Here is the repo. I already have a github.io website for the twcloud org, but I don't know what I should specifically do for this plugin to make it available to the Tiddlyverse.

https://github.com/twcloud/tiddlyweb-sse

Jermolene · 2021-07-14T14:20:03Z

Hi @Arlen22 apologies for all the work on this, but I think we need to make this two separate PRs in order to make ut easier for people to see what's going on:

First a PR that just reverts Add server sent events #5279. GitHub won't do that automatically because there have been subsequent changes, so this will need to be prepared manually
Another PR that adds the new hooks

Many thanks for your help

- Copy in the changes from PR 5853 to disable polling. TiddlyWiki#5853

Arlen22 · 2021-07-14T15:30:03Z

Hi @Arlen22 apologies for all the work on this, but I think we need to make this two separate PRs in order to make ut easier for people to see what's going on:

First a PR that just reverts Add server sent events #5279. GitHub won't do that automatically because there have been subsequent changes, so this will need to be prepared manually

Another PR that adds the new hooks

Many thanks for your help

Ok, I created both PRs, but the second one's branch is forked from the first one's branch, so it has both commits. I'm not entirely sure how that works. It's up to you whether you want to merge only the second one unsquashed so both commits are visible or just merge the first one and then try the second one and if it doesn't work I'll rebase.

#5881 is the second one, which contains both commits.

#5880 is the first one. I listed my actions for the revert in the PR as well as the commit description.

Jermolene · 2021-07-14T16:18:34Z

Thanks @Arlen22 much appreciated. Can this be closed?

Rebase to master and copy in changes from TiddlyWiki#5853.

Arlen22 · 2021-07-14T17:04:14Z

Alright I rebased after #5880 and opened #5882.

Arlen22 · 2021-07-14T19:05:36Z

As I wrote, it was a problem from my nodejs version. I'm using 12.x ... Where there is a default timeout for http connections of 2 minutes. ... The client / browser does open a GET "wiki-change" connection, that shouldn't be closed ... But it was closed from the server. So the client initiated a reconnect. Starting from nodejs version 13 they did remove this 2minute timeout.

@pmario thank you. I was able to reproduce this with the same error in Node 14 with request.setTimeout(10000), so I just added request.setTimeout(0) to the request handler in sse-server.js. This way other connections will be unaffected and I can do it right from the route handler, rather than setting it for the entire server.

do you have a plugin repo already? Can you provide a link?

https://github.com/twcloud/tiddlyweb-sse

Cleanup some code

aaace50

- Move sse-client into the tiddlyweb adapter class itself and call when the status is received. - Generate an initial event id so reconnections work properly even if no events have been emitted yet. - Rename server variable in the status JSON

repeater route fixes

c165cc3

- change name to repeater route - change repeater route to just return eventID

pmario reviewed Jul 6, 2021

View reviewed changes

plugins/tiddlywiki/tiddlyweb/tiddlywebadaptor.js Outdated Show resolved Hide resolved

pmario reviewed Jul 6, 2021

View reviewed changes

core/modules/server/server-sent-events.js Outdated Show resolved Hide resolved

Fix formatting

5b71bb1

- Fix braces in compiled JS by adding them in the source - Fix initJournal casing - Fix spacing

Reformat the other two files

7b89e48

Arlen22 added 2 commits July 6, 2021 23:01

Create tiddlyweb-sse plugin

c1afb18

- Set SyncDisablePolling to a status tiddler so it doesn't get synced. - Add hook to syncer.js - Remove server variable from status and docs. - Move the tiddlyweb related code into the plugin folder. - Nothing done to SSE server code

A few fixes

43e79b8

- Add warning to server side - Format sse-client.js with tabs - Remove check for server variable that I missed.

Arlen22 added 3 commits July 6, 2021 23:26

Set instance variable isPollingDisabled

1f3fac5

Have syncer hook just include self and !err

a5dc494

a couple fixes

d373a18

Move files into own plugin

0da667f

Obviously this isn't going into the PR, just putting it here for now while we work on it.

Jermolene mentioned this pull request Jul 9, 2021

Use server-sent events for notifying changes to browser #1581

Closed

Remove other plugin

ff06ad1

Jermolene mentioned this pull request Jul 14, 2021

Add server sent events #5279

Merged

Arlen22 mentioned this pull request Jul 14, 2021

TW 5.2.0 release imminent - last call for bug fixes and outstanding PRs #5717

Closed

Arlen22 added a commit to Arlen22/TiddlyWiki5 that referenced this pull request Jul 14, 2021

Add changes from "Improvements to SSE (TiddlyWiki#5853)"

ddf07b4

- Copy in the changes from PR 5853 to disable polling. TiddlyWiki#5853

Arlen22 added a commit to Arlen22/TiddlyWiki5 that referenced this pull request Jul 14, 2021

Add syncer status hook and option to disable polling

05f8145

Rebase to master and copy in changes from TiddlyWiki#5853.

Arlen22 mentioned this pull request Jul 14, 2021

Add syncer status hook and option to disable polling #5882

Closed

Arlen22 closed this Jul 14, 2021

pmario mentioned this pull request Mar 23, 2022

[IDEA] Drop support for Internet Explorer after 5.1.23 release #5192

Open

pmario mentioned this pull request Feb 29, 2024

[IDEA] http2 For lazy loading #8017

Open

Improvements to SSE #5853

Improvements to SSE #5853

Conversation

Arlen22 commented Jul 6, 2021

pmario commented Jul 6, 2021 • edited Loading

Arlen22 commented Jul 6, 2021

pmario commented Jul 6, 2021

Arlen22 commented Jul 6, 2021

Arlen22 commented Jul 6, 2021 • edited Loading

Arlen22 commented Jul 6, 2021 • edited Loading

pmario Jul 6, 2021

Choose a reason for hiding this comment

pmario Jul 6, 2021

Choose a reason for hiding this comment

Jermolene commented Jul 6, 2021

pmario commented Jul 6, 2021

pmario commented Jul 6, 2021

Jermolene commented Jul 6, 2021

pmario commented Jul 6, 2021

pmario commented Jul 6, 2021 • edited Loading

Arlen22 commented Jul 6, 2021

Arlen22 commented Jul 6, 2021

Arlen22 commented Jul 7, 2021

Arlen22 commented Jul 7, 2021

pmario commented Jul 7, 2021

pmario commented Jul 8, 2021

pmario commented Jul 8, 2021 • edited Loading

pmario commented Jul 8, 2021

pmario commented Jul 9, 2021

Arlen22 commented Jul 9, 2021

Arlen22 commented Jul 9, 2021

Arlen22 commented Jul 9, 2021

pmario commented Jul 9, 2021 • edited Loading

pmario commented Jul 9, 2021

pmario commented Jul 9, 2021

Arlen22 commented Jul 9, 2021

pmario commented Jul 9, 2021

pmario commented Jul 12, 2021

Arlen22 commented Jul 14, 2021

Jermolene commented Jul 14, 2021

Arlen22 commented Jul 14, 2021 • edited Loading

Jermolene commented Jul 14, 2021

Arlen22 commented Jul 14, 2021

Arlen22 commented Jul 14, 2021 • edited Loading

pmario commented Jul 6, 2021 •

edited

Loading

Arlen22 commented Jul 6, 2021 •

edited

Loading

Arlen22 commented Jul 6, 2021 •

edited

Loading

pmario commented Jul 6, 2021 •

edited

Loading

pmario commented Jul 8, 2021 •

edited

Loading

pmario commented Jul 9, 2021 •

edited

Loading

Arlen22 commented Jul 14, 2021 •

edited

Loading

Arlen22 commented Jul 14, 2021 •

edited

Loading