Race condition when reading the service instance - SW Health Indicator #4906
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
NV-3202 Race condition when reading the service instance - SW Health Indicator
Reproduction StepsThere is a race condition happening in the WS service when reading the The code part where it is happening: The issue should be reproducible when spinning the service (or a few instances). Expected Behaviour
|
djabarovgeorge
commented
Nov 28, 2023
| @@ -18,6 +20,7 @@ export class HealthController { | |||
| const result = await this.healthCheckService.check([ | |||
| async () => this.dalHealthIndicator.isHealthy(), | |||
| async () => this.webSocketsQueueHealthIndicator.isHealthy(), | |||
| async () => this.wsHealthIndicator.isHealthy(), | |||
There was a problem hiding this comment.
Now we will check if the WS server is up.
djabarovgeorge
commented
Nov 28, 2023
Comment on lines
+18
to
+32
|
|
||
| if (!isOnline) { | ||
| return; | ||
| } | ||
|
|
||
| if (command.event === WebSocketEventEnum.RECEIVED) { | ||
| await this.processReceivedEvent(command); | ||
| } | ||
|
|
||
| if (command.event === WebSocketEventEnum.UNSEEN) { | ||
| await this.sendUnseenCountChange(command); | ||
| } | ||
|
|
||
| if (command.event === WebSocketEventEnum.UNREAD) { | ||
| await this.sendUnreadCountChange(command); |
There was a problem hiding this comment.
Small refactor, not related to the PR
djabarovgeorge
commented
Nov 28, 2023
| @@ -127,7 +130,13 @@ export class ExternalServicesRoute { | |||
| } | |||
| } | |||
|
|
|||
| private async connectionExist(command: ExternalServicesRouteCommand) { | |||
| private async connectionExist(command: ExternalServicesRouteCommand): Promise<boolean | undefined> { | |||
| if (!this.wsGateway.server) { | |||
There was a problem hiding this comment.
If the server is not initialized we want to log it.
djabarovgeorge
commented
Nov 28, 2023
| @WebSocketGateway() | ||
| export class WSGateway implements OnGatewayConnection, OnGatewayDisconnect { | ||
| constructor(private jwtService: JwtService, private subscriberOnlineService: SubscriberOnlineService) {} | ||
|
|
||
| @WebSocketServer() | ||
| server: Server; | ||
| server: Server | null; |
There was a problem hiding this comment.
Now we know that there is an edge case where the Server can be nullish.
djabarovgeorge
commented
Nov 28, 2023
Comment on lines
+120
to
+121
| Logger.error('No sw server available to send message', LOG_CONTEXT); | ||
|
|
There was a problem hiding this comment.
If the server is not initialized we want to log it.
djabarovgeorge
commented
Nov 28, 2023
| import { WSGateway } from '../ws.gateway'; | ||
|
|
||
| @Injectable() | ||
| export class WSHealthIndicator extends HealthIndicator implements IHealthIndicator { |
There was a problem hiding this comment.
Will rename in the upcoming PR's in order to make clear what the health indicator is responsible for.
Suggested change
| export class WSHealthIndicator extends HealthIndicator implements IHealthIndicator { | |
| export class WSServerHealthIndicator extends HealthIndicator implements IHealthIndicator { |
…ice-instance-sw-health-indicator
…n-when-reading-the-service-instance-sw-health-indicator # Conflicts: # apps/ws/src/health/health.module.ts # packages/application-generic/src/health/index.ts
djabarovgeorge
deleted the
nv-3202-race-condition-when-reading-the-service-instance-sw-health-indicator
branch
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What change does this PR introduce?
Reproduction Steps
There is a race condition happening in the WS service when reading the server instance sockets. The error message that was noticed has been like Cannot read proprties of sockets of null.
The code part where it is happening:
The issue should be reproducible when spinning the service (or a few instances).
Why was this change needed?
Will validate the WS server health.
Other information (Screenshots)