From 5aee1ca391b76d77d341b7247bbc83514d08742a Mon Sep 17 00:00:00 2001 From: UlyanaAndrukhiv Date: Thu, 5 Dec 2024 12:22:26 +0200 Subject: [PATCH 1/6] Refactored logging the error message for controller --- engine/access/rest/websockets/controller.go | 18 ++++-------------- 1 file changed, 4 insertions(+), 14 deletions(-) diff --git a/engine/access/rest/websockets/controller.go b/engine/access/rest/websockets/controller.go index 07f76c93fba..169dc3b1d58 100644 --- a/engine/access/rest/websockets/controller.go +++ b/engine/access/rest/websockets/controller.go @@ -54,7 +54,7 @@ func (c *Controller) HandleConnection(ctx context.Context) { err := c.configureKeepalive() if err != nil { // TODO: add error handling here - c.logger.Error().Err(err).Msg("error configuring connection") + c.logger.Error().Err(err).Msg("error configuring keepalive connection") c.shutdownConnection() return } @@ -135,13 +135,11 @@ func (c *Controller) writeMessagesToClient(ctx context.Context) error { // if the client is slow or unresponsive. This prevents resource exhaustion // and allows the server to gracefully handle timeouts for delayed writes. if err := c.conn.SetWriteDeadline(time.Now().Add(WriteWait)); err != nil { - c.logger.Error().Err(err).Msg("failed to set the write deadline") - return err + return fmt.Errorf("failed to set the write deadline: %w", err) } err := c.conn.WriteJSON(msg) if err != nil { - c.logger.Error().Err(err).Msg("error writing to connection") - return err + return fmt.Errorf("failed to write message to connection: %w", err) } } } @@ -156,7 +154,6 @@ func (c *Controller) readMessagesFromClient(ctx context.Context) error { for { select { case <-ctx.Done(): - c.logger.Info().Msg("context canceled, stopping read message loop") return ctx.Err() default: msg, err := c.readMessage() @@ -164,18 +161,15 @@ func (c *Controller) readMessagesFromClient(ctx context.Context) error { if websocket.IsCloseError(err, websocket.CloseNormalClosure, websocket.CloseAbnormalClosure) { return nil } - c.logger.Warn().Err(err).Msg("error reading message from client") return fmt.Errorf("failed to read message from client: %w", err) } - baseMsg, validatedMsg, err := c.parseAndValidateMessage(msg) + _, validatedMsg, err := c.parseAndValidateMessage(msg) if err != nil { - c.logger.Debug().Err(err).Msg("error parsing and validating client message") return fmt.Errorf("failed to parse and validate client message: %w", err) } if err := c.handleAction(ctx, validatedMsg); err != nil { - c.logger.Warn().Err(err).Str("action", baseMsg.Action).Msg("error handling action") return fmt.Errorf("failed to handle message action: %w", err) } } @@ -221,7 +215,6 @@ func (c *Controller) parseAndValidateMessage(message json.RawMessage) (models.Ba validatedMsg = listMsg default: - c.logger.Debug().Str("action", baseMsg.Action).Msg("unknown action type") return baseMsg, nil, fmt.Errorf("unknown action type: %s", baseMsg.Action) } @@ -318,9 +311,6 @@ func (c *Controller) keepalive(ctx context.Context) error { case <-pingTicker.C: err := c.conn.WriteControl(websocket.PingMessage, time.Now().Add(WriteWait)) if err != nil { - // Log error and exit the loop on failure - c.logger.Debug().Err(err).Msg("failed to send ping") - return fmt.Errorf("failed to write ping message: %w", err) } } From 17d40b6cb5031d8011f90595aac0f149fb951a0d Mon Sep 17 00:00:00 2001 From: UlyanaAndrukhiv Date: Thu, 5 Dec 2024 12:43:42 +0200 Subject: [PATCH 2/6] Updated HandleConnection to always shut down the connection --- engine/access/rest/websockets/controller.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/engine/access/rest/websockets/controller.go b/engine/access/rest/websockets/controller.go index 169dc3b1d58..ada599571a5 100644 --- a/engine/access/rest/websockets/controller.go +++ b/engine/access/rest/websockets/controller.go @@ -79,8 +79,9 @@ func (c *Controller) HandleConnection(ctx context.Context) { if err = g.Wait(); err != nil { //TODO: add error handling here c.logger.Error().Err(err).Msg("error detected in one of the goroutines") - c.shutdownConnection() } + + c.shutdownConnection() } // configureKeepalive sets up the WebSocket connection with a read deadline From 29fe648fd0aa603eae06816e35dbe44b256bdca2 Mon Sep 17 00:00:00 2001 From: UlyanaAndrukhiv Date: Thu, 5 Dec 2024 13:12:19 +0200 Subject: [PATCH 3/6] Updated behaviour of controller for context canceled --- engine/access/rest/websockets/controller.go | 6 +++--- engine/access/rest/websockets/controller_test.go | 3 +-- 2 files changed, 4 insertions(+), 5 deletions(-) diff --git a/engine/access/rest/websockets/controller.go b/engine/access/rest/websockets/controller.go index ada599571a5..134947bb3cd 100644 --- a/engine/access/rest/websockets/controller.go +++ b/engine/access/rest/websockets/controller.go @@ -122,7 +122,7 @@ func (c *Controller) writeMessagesToClient(ctx context.Context) error { for { select { case <-ctx.Done(): - return ctx.Err() + return nil case msg, ok := <-c.communicationChannel: if !ok { err := fmt.Errorf("communication channel closed, no error occurred") @@ -155,7 +155,7 @@ func (c *Controller) readMessagesFromClient(ctx context.Context) error { for { select { case <-ctx.Done(): - return ctx.Err() + return nil default: msg, err := c.readMessage() if err != nil { @@ -308,7 +308,7 @@ func (c *Controller) keepalive(ctx context.Context) error { for { select { case <-ctx.Done(): - return ctx.Err() + return nil case <-pingTicker.C: err := c.conn.WriteControl(websocket.PingMessage, time.Now().Add(WriteWait)) if err != nil { diff --git a/engine/access/rest/websockets/controller_test.go b/engine/access/rest/websockets/controller_test.go index b1d26ec36ab..32e4a36514e 100644 --- a/engine/access/rest/websockets/controller_test.go +++ b/engine/access/rest/websockets/controller_test.go @@ -242,8 +242,7 @@ func (s *ControllerSuite) TestKeepaliveContextCancel() { // Start the keepalive process with the context canceled err := controller.keepalive(ctx) - s.Require().Error(err) - s.Require().ErrorIs(context.Canceled, err) + s.Require().NoError(err) // Assert expectations s.connection.AssertExpectations(s.T()) // Should not invoke WriteMessage after context cancellation From 7012c3a4ae1292b4e89c4dffb82ed9af3a54cc46 Mon Sep 17 00:00:00 2001 From: Uliana Andrukhiv Date: Thu, 5 Dec 2024 13:18:42 +0200 Subject: [PATCH 4/6] Cleaned up the code Co-authored-by: Andrii Slisarchuk --- engine/access/rest/websockets/controller.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/engine/access/rest/websockets/controller.go b/engine/access/rest/websockets/controller.go index 134947bb3cd..dbd7c678849 100644 --- a/engine/access/rest/websockets/controller.go +++ b/engine/access/rest/websockets/controller.go @@ -125,8 +125,7 @@ func (c *Controller) writeMessagesToClient(ctx context.Context) error { return nil case msg, ok := <-c.communicationChannel: if !ok { - err := fmt.Errorf("communication channel closed, no error occurred") - return err + return fmt.Errorf("communication channel closed, no error occurred") } // TODO: handle 'response per second' limits From 83f070f4f4fb5acee35053c3ade56f36d7518b74 Mon Sep 17 00:00:00 2001 From: UlyanaAndrukhiv Date: Thu, 5 Dec 2024 16:43:44 +0200 Subject: [PATCH 5/6] Updated duration according to comment --- engine/access/rest/websockets/controller_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/engine/access/rest/websockets/controller_test.go b/engine/access/rest/websockets/controller_test.go index 32e4a36514e..ecf8972e4ed 100644 --- a/engine/access/rest/websockets/controller_test.go +++ b/engine/access/rest/websockets/controller_test.go @@ -203,7 +203,7 @@ func (s *ControllerSuite) TestKeepaliveHappyCase() { expectedCalls := 3 // expected 3 ping messages for 30 seconds s.Require().Eventually(func() bool { return len(s.connection.Calls) == expectedCalls - }, 30*time.Second, 1*time.Second, "not all ping messages were sent") + }, time.Duration(expectedCalls)*PongWait, 1*time.Second, "not all ping messages were sent") s.connection.On("Close").Return(nil).Once() controller.shutdownConnection() From f07807e329a60444989ddf464c59c5f598ef551a Mon Sep 17 00:00:00 2001 From: UlyanaAndrukhiv Date: Thu, 5 Dec 2024 18:47:09 +0200 Subject: [PATCH 6/6] Added comment for test according suggestion --- engine/access/rest/websockets/controller_test.go | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/engine/access/rest/websockets/controller_test.go b/engine/access/rest/websockets/controller_test.go index ecf8972e4ed..e6f117d53e4 100644 --- a/engine/access/rest/websockets/controller_test.go +++ b/engine/access/rest/websockets/controller_test.go @@ -128,14 +128,24 @@ func (s *ControllerSuite) TestControllerShutdown() { Topic: dp.BlocksTopic, Arguments: nil, } + msg, err := json.Marshal(requestMessage) + s.Require().NoError(err) + // This is due to how the mock library compares arguments: it requires the + // pointers passed in `On` to match the exact memory address of the pointer + // passed at runtime. Since these pointers are not guaranteed to be the same, + // strict matching (`&msg`) will fail. + // + // Using `mock.Anything` bypasses this strict matching. The `Run` function + // then allows us to simulate the behavior of `ReadJSON` by taking the argument + // provided during the method call (a dynamically allocated `*json.RawMessage`) + // and setting its value (`*reqMsg = msg`). This ensures that the behavior + // mimics the real method while avoiding argument mismatch issues. s.connection. On("ReadJSON", mock.Anything). Run(func(args mock.Arguments) { reqMsg, ok := args.Get(0).(*json.RawMessage) s.Require().True(ok) - msg, err := json.Marshal(requestMessage) - s.Require().NoError(err) *reqMsg = msg }). Return(nil).