-
Notifications
You must be signed in to change notification settings - Fork 0
Adding re connection management #53
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
@@ -6,7 +6,9 @@ namespace ReactiveXComponent.Connection | |||
public interface IXCSession : IDisposable | |||
{ | |||
bool IsOpen { get; } | |||
event EventHandler SessionOpened; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not Action instead of EventHandler ? It seems that you don't use the sender and EventArgs are empty most of the time
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll refactor this in the next version as I try to avoid any impact on the modules using this Api (like Client Api generated by the XCStudio).
{ | ||
_endpoint = endpoint; | ||
_timeout = timeout; | ||
|
||
_maxRetries = maxRetries; | ||
_retryInterval = (retryInterval != null) ? retryInterval.Value : TimeSpan.FromSeconds(5); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It could be interesting to increment the _retryInterval depending on _currentRetryCount
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the JS API we decided to leave the reconnection strategy up to the user... what is the point of implementing that on the API side if it is probably application specific?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Add tests
- Where is the retry interval and max retries set? I only see it at the Websocket/Rabbit level, but not on the session level. How is the user supposed to set them?
{ | ||
_endpoint = endpoint; | ||
_timeout = timeout; | ||
|
||
_maxRetries = maxRetries; | ||
_retryInterval = (retryInterval != null) ? retryInterval.Value : TimeSpan.FromSeconds(5); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the JS API we decided to leave the reconnection strategy up to the user... what is the point of implementing that on the API side if it is probably application specific?
event EventHandler SessionClosed; | ||
event EventHandler<System.IO.ErrorEventArgs> ConnectionError; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On the JS API we called it DisconnectionError because what we are handling here is the case of a disconnection after connection.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This event is raised when we encounter a problem while connecting. That's why it's called ConnectionError. You can refer to WebSocketClient.Init() method.
@nicolaserny @mbarekh @marwathlithi I think we need to adopt a common approach to be followed by all APIs, instead of having them diverging. |
I agree with @maurelio1234 that all our APIs need to follow a common approach. This pull request was created to implement a reconnection management for the Websocket client Api (cf. issue #46 ) |
Cancelling this PR as it's too old. |
closes #46