Timeout should be parameter #21
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
# problem 5.seconds.fromNow() is too short for slack webSockecet Client. Usually many server is use 20 second ping/pong setting, so it should be longer than that. Otherwise it throw SocketConnectionError Slack send ping pong like 20 second interval, so for websockets, this should be like 1.minute.fromNow(). # Fix This PR made deadline (timeout second) to be parameter. # Result I’ve tested and 5.seconds throw SocketConnectionError, 60.seconds.fromNow() can keep connection at least 2-3 hours # Merge-ability This adds timeout parameter on WebSocket Class but it has default value so it should not make API conflict or changes. Should be safe to tag it.
|
The only worry I have is naming - is Other than that good to go :) |
|
I think maybe |
|
Any other suggestions? |
|
OK lemme change |
timeout -> connectionTimeout
|
changed @paulofaria @Danappelxx |
tomohisa
pushed a commit
to ZewoGraveyard/WebSocketClient
that referenced
this pull request
Oct 18, 2016
# problem 5.seconds.fromNow() is too short for slack webSockecet Client. Usually many server is use 20 second ping/pong setting, so it should be longer than that. Otherwise it throw SocketConnectionError Slack send ping pong like 20 second interval, so for websockets, this should be like 1.minute.fromNow(). After the test, slack still does not work with 60.seconds, so I would like to pass own value to WebSocketClient # Fix This PR made deadline (timeout second) to be parameter. Default timeout parameter is now `60.seconds` ZewoGraveyard/WebSocket#21 Changed on WebSocket Package, but WebSocetClient does not have pass the value, so User could not set own parameter. This change allow to set own parameter if client wish # Result I’ve tested and 5.seconds throw SocketConnectionError, 60.seconds.fromNow() can keep connection at least 2-3 hours. For production maybe I will use 10.minutes (But it depends on server so I would like it to pass my own value) # Merge-ability This adds connectionTimeout parameter on WebSocketClient Class but it has default value nil, and if its nil, WebSocketClient use WebSocket default value. so it should not make API conflict or changes. Should be safe to tag it.
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.
problem
5.seconds.fromNow() is too short for slack webSockecet Client.
Usually many server is use 20 second ping/pong setting, so it should be
longer than that. Otherwise it throw SocketConnectionError
Slack send ping pong like 20 second interval, so for websockets, this
should be like 1.minute.fromNow().
Fix
This PR made deadline (timeout second) to be parameter. Default timeout parameter is now
60.secondsResult
I’ve tested and 5.seconds throw SocketConnectionError,
60.seconds.fromNow() can keep connection at least 2-3 hours
Merge-ability
This adds connectionTimeout parameter on WebSocket Class but it has default value
so it should not make API conflict or changes. Should be safe to tag it.