rpcsrv: enforce WS connection close on test cleanup #3398
Merged
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
AnnaShaleva
force-pushed
the
enforce-testws-close
branch
from
fc3e9dc to
a1ca8ab
Compare
Do not wait until wsReader routine gracefully finishes its work before WS connection close. Instead, firstly close the connection, and after that wait for proper wsReader exit. It's a harsh way, but I don't have any other options to try, because wsReader routine hangs on `ws.ReadMessage()` operation for more than ReadDeadline (more than 5 seconds) during test cleanup which results in the test timeout. Close #3378. Signed-off-by: Anna Shaleva <[email protected]>
AnnaShaleva
force-pushed
the
enforce-testws-close
branch
from
a1ca8ab to
e5c919f
Compare
AnnaShaleva
changed the title
|
Let's firstly see if it helps. If not, then I'm likely wrong about the reason of timeout. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #3398 +/- ##
==========================================
- Coverage 84.78% 84.78% -0.01%
==========================================
Files 331 331
Lines 44972 44972
==========================================
- Hits 38131 38128 -3
- Misses 5328 5330 +2
- Partials 1513 1514 +1 ☔ View full report in Codecov by Sentry. |
|
Nice, Coverage tests are PASSed, but the Codecove uploading step itself fails: |
|
OK, it may happen due to the fact that this PR is based on a very fresh master, and Codecov results are not yet properly uploaded for the base. Not critical for us, may happen from time to time. |
AliceInHunterland
approved these changes
Apr 4, 2024
AnnaShaleva
commented
Apr 4, 2024
| ws.Close() | ||
| <-readerToExitCh |
There was a problem hiding this comment.
The only thing I'm worried about is the fact that we have a similar usage pattern in our WS client. It's not exactly similar, because it's wsWriter goroutine who is responsible for connection closing, but still, I don't quite understand the reason of WS read process hanging given the fact that we have timeout set. But we have tests for WS client closing process, and it passes on Windows, so likely there's no problem in real code.
roman-khimov
approved these changes
Apr 4, 2024
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.
Do not wait until wsReader routine gracefully finishes its work before WS connection close. Instead, firstly close the connection, and after that wait for proper wsReader exit.
It's a harsh way, but I don't have any other options to try, because wsReader routine hangs on
ws.ReadMessage()operation for more than ReadDeadline (more than 5 seconds) during test cleanup which results in the test timeout.Close #3378.