Fix network recovering #62
Merged
Conversation
This file contains hidden or 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
Codecov Report
@@ Coverage Diff @@
## 1-dev #62 +/- ##
==========================================
+ Coverage 82.60% 82.67% +0.06%
==========================================
Files 34 34
Lines 1696 1743 +47
Branches 201 205 +4
==========================================
+ Hits 1401 1441 +40
- Misses 272 278 +6
- Partials 23 24 +1
Continue to review full report at Codecov.
|
Aschen
reviewed
Sep 22, 2020
Contributor
Aschen
left a comment
Aschen
left a comment
There was a problem hiding this comment.
My dear c#, long time no see but looks good to me 👍
alexandrebouthinon
approved these changes
Sep 25, 2020
Merged
scottinet
added a commit
that referenced
this pull request
Sep 25, 2020
# [1.0.4](https://github.com/kuzzleio/sdk-csharp/releases/tag/1.0.4) (2020-09-25) #### Bug fixes - [ [#62](#62) ] Fix network recovering ([scottinet](https://github.com/scottinet)) ---
scottinet
added a commit
that referenced
this pull request
Sep 29, 2020
Fix a number of problems when handling disconnections, and with asynchronous tasks in general. When `AbstractProtocol.AutoRecover` is set to true: * requests sent through the network would never be resolved when the connection is lost before they receive a response. This means that clients awaiting for a response would freeze forever. With this PR, those tasks are correctly rejected with a `ConnectionLost` exception (we cannot queue them: as far as the SDK is concerned, it cannot know if these requests were received and processed by Kuzzle or not) * there was a race condition when triggering the threaded recovering process where, in rare cases, 2 reconnection attempts would run in parallel, doubling the number of opened sockets * Race conditions could occur and corrupt the requests cache, or freeze the SDK because of an unhandled exception, because of an incorrect use of `lock` (this keyword has no impact when multiple accesses are made to the same "locked" object within the same thread) => semaphores are now used to circumvent the problem * Requests tasks were incorrectly configured, making their resolution synchronous. This means that resolving tasks in the SDK event handlers (which manage network state changes) could prevent them to finish in a timely fashion (or... ever) because they would synchronously trigger awaiting code * Save our standard code style directly in the solution to share it to whomever works on the project
scottinet
added a commit
that referenced
this pull request
Sep 29, 2020
# Description Fix a number of problems when handling disconnections, and with asynchronous tasks in general. ## Network disconnection When `AbstractProtocol.AutoRecover` is set to true: * requests sent through the network would never be resolved when the connection is lost before they receive a response. This means that clients awaiting for a response would freeze forever. With this PR, those tasks are correctly rejected with a `ConnectionLost` exception (we cannot queue them: as far as the SDK is concerned, it cannot know if these requests were received and processed by Kuzzle or not) * there was a race condition when triggering the threaded recovering process where, in rare cases, 2 reconnection attempts would run in parallel, doubling the number of opened sockets ## Asynchronous tasks * Race conditions could occur and corrupt the requests cache, or freeze the SDK because of an unhandled exception, because of an incorrect use of `lock` (this keyword has no impact when multiple accesses are made to the same "locked" object within the same thread) => semaphores are now used to circumvent the problem * Requests tasks were incorrectly configured, making their resolution synchronous. This means that resolving tasks in the SDK event handlers (which manage network state changes) could prevent them to finish in a timely fashion (or... ever) because they would synchronously trigger awaiting code # Boyscout * Save our standard code style directly in the solution to share it to whomever works on the project
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.
Description
Fix a number of problems when handling disconnections, and with asynchronous tasks in general.
Network disconnection
When
AbstractProtocol.AutoRecoveris set to true:ConnectionLostexception (we cannot queue them: as far as the SDK is concerned, it cannot know if these requests were received and processed by Kuzzle or not)Asynchronous tasks
lock(this keyword has no impact when multiple accesses are made to the same "locked" object within the same thread) => semaphores are now used to circumvent the problemBoyscout