-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
WIP: Websocket replacement for Eventstream #20543
Conversation
if (!this.webSocket) return; | ||
const oldWebSocket = this.webSocket; | ||
this.webSocket = null; | ||
oldWebSocket.close(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For resilience on flaky connections, we want to reconnect on abnormal closure (should be e.code != 1000
), after a small timeout of like 1s, ideally giving up after a certain period of failure, like 30s. I do have some old cody lying around for this, can likely share later.
So the problem I'm having right now is that the Dom update after the notification event has been received isn't happening and I just don't understand why. Hence all the logging there. |
- Since go-gitea#20108 we have two version of the notification bell, one for mobile the other for non-mobile. However the code only accounts for one notification count and thus was only updating the non-mobile one. - This code fixes that by applying the code for all `.notification_count`s. - Frontport will be in go-gitea#20543
Note to frontport #20544 into this PR. |
- Since #20108 we have two version of the notification bell, one for mobile the other for non-mobile. However the code only accounts for one notification count and thus was only updating the non-mobile one. - This code fixes that by applying the code for all `.notification_count`s. - Frontport will be in #20543
Signed-off-by: Andrew Thornton <art27@cantab.net>
Signed-off-by: Andrew Thornton <art27@cantab.net>
Signed-off-by: Andrew Thornton <art27@cantab.net>
OK... now the next thing to do is to either merge the websocket and eventsource sharedworkers or just kill the eventsource sharedworker. Realistically I think everything that supports eventsources also supports websockets so it's probably not worth keeping both around unless there's some reason why websocket cannot be supported on the server end but eventsource can be. (Is there?) I think we could also make a COMET backend also work too. OK looking at things like Socket.io and sockjs there are still cases where WebSocket won't work because of proxies so we will still need to have a fall back. However, in both of these cases the go server backends appear in a poor state so we need to just do the work to implement this ourselves I think. Now the EventSource would still work as the SSE in those cases so we just need the POST part to work and some way to detect when the WebSocket just isn't going to work. |
I'd just kill the EventSource. As I see it, it's a graceful degradation if WebSocket doesn't work because the features that rely on it are not something I would consider critical. |
As for reconnection behaviour, you can take a hint from sockette or even use it as it's a tiny wrapper. |
Actually I may have some SharedWorker WebSocket code with reconnects to share as I'm working on one currently. Still need to clean it up thought. |
Here's my rather terse script of a SharedWorker that processes JSON on a WebSocket and does the reconnecting. It's based on sockette with all the cruft removed. I'm not sure if I think it's preferable to do as little work as possible in the worker because workers are a pain in the ass to debug. Just have the worker passing the JSON and have the main thread do the rest. onconnect = async ({ports: [port]}) => {
let ws;
function open() {
return new Promise((resolve) => {
ws = new WebSocket(`${location.origin.replace(/^http/, "ws")}/ws`);
ws.addEventListener("message", ({data}) => port.postMessage(JSON.parse(data)));
ws.addEventListener("open", resolve);
ws.addEventListener("close", () => setTimeout(open, 1000));
ws.addEventListener("error", (e) => e?.code === "ECONNREFUSED" && setTimeout(open, 1000));
});
}
port.addEventListener("message", ({data}) => {
ws.send(JSON.stringify(data));
});
await open();
port.start();
}; |
@@ -365,6 +365,7 @@ func RegisterRoutes(m *web.Route) { | |||
}, reqSignOut) | |||
|
|||
m.Any("/user/events", routing.MarkLongPolling, events.Events) | |||
m.Any("/user/websocket", routing.MarkLongPolling, events.Websocket) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this cookie-authenticated? If not, it should be.
CheckOrigin: func(r *http.Request) bool { | ||
origin := r.Header["Origin"] | ||
if len(origin) == 0 { | ||
return true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return true | |
return false |
Origin header will be present even on first-party requests, so we can require it.
return true | ||
} | ||
|
||
return u.Host == appURLURL.Host |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure this is safe, it should probably be stricter and include protocol and port as well. Thought we could also lean more on the cookie header. When the cookie has SameSite=strict, it could technically serve as the only authentification mechanism on the websocket. Thought, origin check is always good to have.
|
||
func readMessagesFromConnToChan(conn *websocket.Conn, messageChan chan readMessage) { | ||
defer func() { | ||
close(messageChan) // Please note: this has to be within a wrapping anonymous func otherwise it will be evaluated when creating the defer |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting, I didn't know this. Is this evaluation behaviour something specific to the close
builtin?
Would still recommend using the SharedWorker code from #20543 (comment), or if SharedWorker proves too hard (it certainly sucks to debug SharedWorker in Chrome or Firefox), websocket could be done on main thread as well (with similar websocket init code), but it'll eat more resources, both server- and client-side. |
Very much work-in-progress attempt at converting the eventstream into a websocket.
The idea would be to use the reader to allow for more notifications to be watched and so on.