feat: Make workspace watching realtime instead of polling #4922
Conversation
This was leading to performance issues on the frontend, where the page should only be rendered if changes occur. While this could be changed on the frontend, it was always the intention to make this socket ~realtime anyways.
|
|
||
| _ = sendEvent(ctx, codersdk.ServerSentEvent{ | ||
| Type: codersdk.ServerSentEventTypePing, | ||
| }) |
There was a problem hiding this comment.
Is this needed because ServerSentEventSender() doesn't send the first ping until the timeout has been hit once? Should we "prime" it in there instead?
There was a problem hiding this comment.
I think we want the caller to indicate readiness by sending the ping themselves.
If we don't a race can occur where the client's request has been completed but subscribe hasn't been triggered yet. We could fix this by moving the initialization below the subscribe and making it send a ping, which seems more reasonable.
Thoughts?
There was a problem hiding this comment.
Seems like this is a bit trickier than I thought... because we use the sendEvent callback in the Subscribe function, we can't initialize after safely. Seems like we'll have to keep it the way it is, but I'll add a comment.
There was a problem hiding this comment.
I added a comment and will leave it as-is for now because nothing comes to mind as being cleaner... if you have ideas leave em here and I'll implement!
There was a problem hiding this comment.
Hmm, I don't fully understand the race scenario, but I just realized that the httpapi.Write above (if err) could be problematic since we've already primed rw for server side events in httpapi.ServerSentEventSender (and started a timer for pings). So it might be a good idea to change things around a bit 👍🏻. If the subscription fails, I believe this handler wouldn't return until the first SSE auto-ping has been sent (because of the defer <-chan above).
There was a problem hiding this comment.
Fixed that error case! Goooood catch
|
@mafredri I also think this is why your CPU usage was so high on the workspace page since the UI had to refresh every second. |
…mments to workspace events
This was leading to performance issues on the frontend, where the page should only be rendered if changes occur. While this could be changed on the frontend, it was always the intention to make this socket ~realtime anyways.