dice icon indicating copy to clipboard operation
dice copied to clipboard

Handle Q.WATCH updates in a single event loop for Websockets

Open psrvere opened this issue 1 year ago • 2 comments

Closes #1153

In PR #1090 Q.WATCH updates were handled by processQwatchUpdates run in a goroutine. Based on this review comment, this PR moves this handling to a single event loop. It

  • adds subscriptionChan and subscribedClients to WebsocketServer
  • runs listenForSubscriptions and processQwatchUpdates as part of WebsocketServer.Run()
  • shutsdown above two goroutines through both shutdownChan (called by abort command) and ctx.Done channel

This PR also

  • adds ReadResponse function in setup.go to read Q.WATCH updates
  • adds two more integration tests TestQWatchWithMultipleClients and TestQWatchWithMultipleClientsAndQueries
  • changes key names of all tests in qwatch_test.go to be unique so that there is no interference with other tests. Since cancelling subscription (Q.UNWATCH) is not supported yet, any following tests that changes the key also triggers Q.WATCH updates. This is a known issue and will be fixed as part of #1154

This PR also fixes following bugs:

  • Data races accessing conn.WriteMessage. Fix: added a mutex
  • Any error in processing Q.WATCH updates resulted in returning processQwatchUpdates goroutine. Fix: It skips update when it can't process it and logs error.
  • RunWebsocketServer in setup.go was relying on both cancelling parent context to shutdown queryManager goroutine and abort command to shutdown shardManager. Fix: Made it consistent - everythings is gracefully shutdown by abort command

psrvere avatar Oct 21 '24 10:10 psrvere

@lucifercr07 - Please review.

psrvere avatar Oct 21 '24 10:10 psrvere

Have we done any benchmarking for the Q.Watch and do we want to check if we are not regressing anything with the above change?

arpit1912 avatar Oct 25 '24 07:10 arpit1912

Hey @psrvere could you please resolve the conflicts?

JyotinderSingh avatar Nov 08 '24 16:11 JyotinderSingh

@JyotinderSingh - Resolved. Please review.

psrvere avatar Nov 11 '24 12:11 psrvere

Closing this as QWATCH has been deprioritised.

psrvere avatar Dec 03 '24 07:12 psrvere