From 48256721c434c2a95d372292d61b7ce97d0cf368 Mon Sep 17 00:00:00 2001 From: Agniva De Sarker Date: Sat, 9 May 2020 08:45:03 +0530 Subject: [PATCH] MM-24611: Fix flaky test TestDeletePreferencesWebsocket (#14399) After registering the conn in the hub, we proceeded to send a direct message to the user. We had changed it to send the direct message in the same hub goroutine that handles the registration. This was the correct behavior and fixes chances of having panics due to sending to closed channels. However, often fixing something unearths some deeper underlying bug. This was such a case :) The issue was that register channel had a buffer size of 1. And we were sending a direct message after registration. In the code to send direct message, we were checking if the user has been registered or not, and if not, then skip it. Therefore, since the register channel buffer was 1, it could very well be that the select case would pick up the direct message send case first - in which case it would not have been registered, and therefore no hello message would be sent. The fix is to unbuffer the register and unregister channels. There does not seem to be a valid reason to make these buffered channels. They are meant to be synchronous operations, because the code following them assumes that the user has been registered. While here, we also remove all the time.Sleeps before waiting on the Response channel because they are not required at all. Waiting on a channel is already blocking. Co-authored-by: Ben Schumacher --- api4/preference_test.go | 1 - api4/websocket_test.go | 11 ----------- app/web_hub.go | 4 ++-- 3 files changed, 2 insertions(+), 14 deletions(-) diff --git a/api4/preference_test.go b/api4/preference_test.go index b7e6aa4d13..8c98f0f890 100644 --- a/api4/preference_test.go +++ b/api4/preference_test.go @@ -364,7 +364,6 @@ func TestDeletePreferencesWebsocket(t *testing.T) { require.Nil(t, err) WebSocketClient.Listen() - time.Sleep(300 * time.Millisecond) wsResp := <-WebSocketClient.ResponseChannel require.Equal(t, model.STATUS_OK, wsResp.Status, "should have responded OK to authentication challenge") diff --git a/api4/websocket_test.go b/api4/websocket_test.go index 16dae3c6f3..4716064103 100644 --- a/api4/websocket_test.go +++ b/api4/websocket_test.go @@ -32,22 +32,18 @@ func TestWebSocket(t *testing.T) { WebSocketClient.Listen() - time.Sleep(300 * time.Millisecond) resp := <-WebSocketClient.ResponseChannel require.Equal(t, resp.Status, model.STATUS_OK, "should have responded OK to authentication challenge") WebSocketClient.SendMessage("ping", nil) - time.Sleep(300 * time.Millisecond) resp = <-WebSocketClient.ResponseChannel require.Equal(t, resp.Data["text"].(string), "pong", "wrong response") WebSocketClient.SendMessage("", nil) - time.Sleep(300 * time.Millisecond) resp = <-WebSocketClient.ResponseChannel require.Equal(t, resp.Error.Id, "api.web_socket_router.no_action.app_error", "should have been no action response") WebSocketClient.SendMessage("junk", nil) - time.Sleep(300 * time.Millisecond) resp = <-WebSocketClient.ResponseChannel require.Equal(t, resp.Error.Id, "api.web_socket_router.bad_action.app_error", "should have been bad action response") @@ -55,23 +51,19 @@ func TestWebSocket(t *testing.T) { req.Seq = 0 req.Action = "ping" WebSocketClient.Conn.WriteJSON(req) - time.Sleep(300 * time.Millisecond) resp = <-WebSocketClient.ResponseChannel require.Equal(t, resp.Error.Id, "api.web_socket_router.bad_seq.app_error", "should have been bad action response") WebSocketClient.UserTyping("", "") - time.Sleep(300 * time.Millisecond) resp = <-WebSocketClient.ResponseChannel require.Equal(t, resp.Error.Id, "api.websocket_handler.invalid_param.app_error", "should have been invalid param response") require.Equal(t, resp.Error.DetailedError, "", "detailed error not cleared") WebSocketClient.UserTyping(th.BasicChannel.Id, "") - time.Sleep(300 * time.Millisecond) resp = <-WebSocketClient.ResponseChannel require.Nil(t, resp.Error) WebSocketClient.UserTyping(th.BasicPrivateChannel2.Id, "") - time.Sleep(300 * time.Millisecond) resp = <-WebSocketClient.ResponseChannel require.Equal(t, resp.Error.Id, "api.websocket_handler.invalid_param.app_error", "should have been invalid param response") require.Equal(t, resp.Error.DetailedError, "", "detailed error not cleared") @@ -96,7 +88,6 @@ func TestWebSocketEvent(t *testing.T) { WebSocketClient.Listen() - time.Sleep(300 * time.Millisecond) resp := <-WebSocketClient.ResponseChannel require.Equal(t, resp.Status, model.STATUS_OK, "should have responded OK to authentication challenge") @@ -175,7 +166,6 @@ func TestCreateDirectChannelWithSocket(t *testing.T) { defer WebSocketClient.Close() WebSocketClient.Listen() - time.Sleep(300 * time.Millisecond) resp := <-WebSocketClient.ResponseChannel require.Equal(t, resp.Status, model.STATUS_OK, "should have responded OK to authentication challenge") @@ -272,7 +262,6 @@ func TestWebSocketStatuses(t *testing.T) { defer WebSocketClient.Close() WebSocketClient.Listen() - time.Sleep(300 * time.Millisecond) resp := <-WebSocketClient.ResponseChannel require.Equal(t, resp.Status, model.STATUS_OK, "should have responded OK to authentication challenge") diff --git a/app/web_hub.go b/app/web_hub.go index 935ab2da5d..fd10126db2 100644 --- a/app/web_hub.go +++ b/app/web_hub.go @@ -53,8 +53,8 @@ type Hub struct { func (a *App) NewWebHub() *Hub { return &Hub{ app: a, - register: make(chan *WebConn, 1), - unregister: make(chan *WebConn, 1), + register: make(chan *WebConn), + unregister: make(chan *WebConn), broadcast: make(chan *model.WebSocketEvent, broadcastQueueSize), stop: make(chan struct{}), didStop: make(chan struct{}),