Auto reconnect RTM by Roach · Pull Request #297 · slackapi/python-slack-sdk

Roach · 2018-03-03T01:01:05Z

Summary

Adds auto_reconnect to RTM

Requirements (place an `x` in each `[ ]`)

I've read and understood the Contributing Guidelines and have done my best effort to follow them.
I've read and agree to the Code of Conduct.

codecov · 2018-03-03T01:28:17Z

Codecov Report

Merging #297 into master will increase coverage by 12.6%.
The diff coverage is 71.73%.

@@            Coverage Diff             @@
##           master     #297      +/-   ##
==========================================
+ Coverage   59.82%   72.43%   +12.6%     
==========================================
  Files          10       10              
  Lines         341      370      +29     
  Branches       62       66       +4     
==========================================
+ Hits          204      268      +64     
+ Misses        121       92      -29     
+ Partials       16       10       -6

Impacted Files	Coverage Δ
slackclient/client.py	`51.66% <60%> (+15.15%)`	⬆️
slackclient/server.py	`82.14% <73.17%> (+21.84%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bca6bcc...2f5be5e. Read the comment docs.

…ct call

aoberoi · 2018-03-13T23:53:06Z

slackclient/client.py

            result = json.loads(response_body)
-        except ValueError as json_decode_error:
-            raise ParseResponseError(response_body, json_decode_error)
-        if self.server:


just noticed that this check didn't make it over. was it superfluous in the first place? can self.server ever not be set?

@aoberoi I don't think there's a way it wouldn't be set...

It's set when the client is initialized:
https://github.com/slackapi/python-slackclient/blob/master/slackclient/client.py#L30-L33

…ws web socket bug: https://bugs.python.org/issue32394

aoberoi · 2018-03-19T18:20:19Z

tests/test_server.py


-def test_server(server):
+def test_server():
+    server = Server("valid_token", connect=False, )


trailing comma cool here?

aoberoi · 2018-03-19T18:21:04Z

tests/test_server.py

+def test_server():
+    server = Server("valid_token", connect=False, )
    assert type(server) == Server
+    assert server == "valid_token"


the server is coerced into a string? is that a useful feature?

No idea. It's a Preexisting Feature™

I've never used it, but I'm sure someone has at some point during the history of this project.

aoberoi · 2018-03-19T18:21:59Z

tests/test_server.py

-    assert userbyname != "someotheruser"
+    user_by_name = server.users.find('fakeuser')
+    assert type(user_by_name) == User
+    assert user_by_name == "fakeuser"


Users are coerced into a string? is that a useful feature?

I'm especially worried about this because we'd like to de-emphasize programmatic usage of username.

this is from a refactor of the "SearchList" class that allowed searching through the client's cache by username or user ID. I just cleaned up the names for readability.

aoberoi · 2018-03-19T18:24:03Z

tests/test_server.py

-    userbyid = server.users.find('invaliduser')
-    assert type(userbyid) != User
+    user_by_id = server.users.find('invaliduser')
+    assert type(user_by_id) != User


okay, its type isn't a user, but what is the type? maybe that's what the assertion should check.

Made it check for None instead 👍

aoberoi · 2018-03-19T18:25:30Z

tests/test_server.py

+            json={"ok": False}
+        )
+
+        with pytest.raises(SlackConnectionError) as e:


i'm not sure what this is meant to do. how i'm currently reading this: "when a SlackConnectionError happens, ping the server". what does this accomplish other than swallowing the error?

In order to write assertions about raised exceptions, you can use pytest.raises as a context manager

https://docs.pytest.org/en/latest/assert.html#assertions-about-expected-exceptions

okay, but there's no assertion about the error in the body

maybe i misunderstood. server.ping() causes the error, then wrapping it in pytest.raises(ErrorName) asserts that the error occurred and it is of that type?

Changed the call to rtm_connect instead of ping, to more accurately reflect what it's testing

aoberoi · 2018-03-19T18:28:14Z

tests/test_server.py

+        server.rtm_connect(auto_reconnect=True, reconnect=True, use_rtm_start=False)
+
+        for call in rsps.calls:
+            assert call.request.url in [


can you explain how this test works? i don't see how the timeout could have been hit.

Added more context to the test

not sure i understand. what i see is connecting when the last_connected_at time is now will immediately cause another connection attempt. that sounds like the opposite of what your comment says should happen.

Yasumoto · 2018-03-20T00:02:24Z

slackclient/client.py

        try:
            self.server.rtm_connect(use_rtm_start=with_team_state, **kwargs)
-            return True
+            if self.server.connected:


return self.server.connected

Yasumoto · 2018-03-20T00:03:28Z

slackclient/client.py

        response_body = self.server.api_call(method, timeout=timeout, **kwargs)
        try:
            result = json.loads(response_body)
-        except ValueError as json_decode_error:


Keep this up here so it's more clear what Errors you handle for which lines

It was still there, just moved way down into the wrong scope...

Yasumoto · 2018-03-20T00:04:55Z

slackclient/client.py

            if method == 'im.open':
                if "ok" in result and result["ok"]:
                    self.server.attach_channel(kwargs["user"], result["channel"]["id"])
+            # If the call was for a `group` or `im` endpoint, attach the group data


I've found that inline comments like this are some combination of three things:

A good sign to break out into a separate function

Suggest better naming of variables

Superfluous and potentially going to be out-of-date after a refactor

Realized these could just reuse server.parse_channel_data is they were passed as a single-item array

Yasumoto · 2018-03-20T00:09:56Z

slackclient/server.py

+            elif self.reconnect_attempt > 0:
+                # Back off after the the first attempt
+                if (time.time() - self.last_connected_at) < 180:
+                    # Random offset to prevent stampeding reconnects of multiple RTM clients


Maybe call the variable backoff_offset_multiplyer and then you'll be covered and don't need the comment

Yasumoto · 2018-03-20T00:10:34Z

slackclient/server.py

+            if self.reconnect_attempt == 5:
+                raise SlackConnectionError("RTM connection failed, reached max reconnects.")
+            elif self.reconnect_attempt > 0:
+                # Back off after the the first attempt


I'd add some debug logging in this chunk of the code like you did below, that also "cheats" and gives you a chance to add a little more documentation inline with the code

Yasumoto · 2018-03-20T00:14:39Z

tests/test_server.py

+            json=rtm_start_fixture
+        )
+
+        server.rtm_connect(auto_reconnect=True)


I think you want some tests that do rsps.add multiple times, so you can fail a few times and ensure you reconnect after N tries, right? (I might've missed this tho).

Essentially have ~3 rsps.add, the first two return a 4xx and then you get a 200 for the third. That'll prove your reconnect handles errors properly 💪

This reconnect logic only tests for when the socket is disconnected, then reconnected. It's not handling connection retries. That's part of another project 😄

Yasumoto · 2018-03-20T00:15:12Z

tests/test_server.py

+
+
+def test_rtm_reconnect_timeout_recently_connected(server, rtm_start_fixture):
+    # If reconnected recently, server must wait to reconnect and increment the counter


Feel free to make these docstrings to tie these to the function for some python-y tooling.

generica · 2018-03-20T00:31:51Z

slackclient/server.py

+        # If this is an auto reconnect, rate limit reconnect attempts
+        if self.auto_reconnect and reconnect:
+            # Raise a SlackConnectionError after 5 retries within 3 minutes
+            if self.reconnect_attempt == 5:


This feels like a value that shouldn't be hidden down here in the guts?

I agree, but I'm not quite sure where else it could go 🤔

generica · 2018-03-20T00:37:10Z

slackclient/server.py

+                    # Random offset to prevent stampeding reconnects of multiple RTM clients
+                    offset = random.randint(1, 4)
+                    reconnect_timeout = (offset * self.reconnect_attempt * self.reconnect_attempt)
+                    logging.debug("Reconnecting in {} seconds".format(reconnect_timeout))


logging.debug("Reconnecting in %d seconds", reconnect_timeout)

* Added reconnect logic to RTM client * Added a lot more tests * updated RTM test fixture * Disable testing for 3.6 on Windows until 3.6.5 release due to a windows web socket bug: https://bugs.python.org/issue32394

Roach added 7 commits February 27, 2018 16:06

Added file upload and regenerated docs

cce6528

Fixed JSON encoding

0db0587

Merge branch 'master' of github.com:slackapi/python-slackclient

0c16ec5

Merge branch 'master' of github.com:slackapi/python-slackclient

659ea70

Merge branch 'master' of github.com:slackapi/python-slackclient

068e735

Merge branch 'master' of github.com:slackapi/python-slackclient

2f29719

Add auto_reconnect to RTM client

8b19db6

Roach mentioned this pull request Mar 5, 2018

RTM "socket.error: [Errno 32] Broken pipe" while receiving user status bulk updates #214

Closed

8 tasks

Roach added 4 commits March 13, 2018 14:01

Added reconnect logic to RTM client

db37a05

code tidying, added random offset for reconnects

c9d8dee

Fix range of random offset, remove unnecessary attribute from reconne…

fc63597

…ct call

Cleaned up client.py and added comments

f336ded

aoberoi reviewed Mar 13, 2018

View reviewed changes

Roach and others added 6 commits March 14, 2018 11:02

Added server tests, updated ram fixture

40d454d

Disable testing for 3.6 on Windows until 3.6.5 release due to a windo…

57cfd86

…ws web socket bug: https://bugs.python.org/issue32394

Added more tests

e155282

Added reconnect to the docs

cf2427a

Removed 3.6 X86 windows test env

d6dc692

Added tests

11c0fa4

Roach added RTM labels Mar 18, 2018

Roach added this to the 1.x milestone Mar 18, 2018

Roach self-assigned this Mar 18, 2018

aoberoi reviewed Mar 19, 2018

View reviewed changes

Roach added 3 commits March 19, 2018 14:21

updated reconnect comment

70200fa

improved tests

360065e

fixed tests

074331b

Yasumoto reviewed Mar 20, 2018

View reviewed changes

generica reviewed Mar 20, 2018

View reviewed changes

Roach added 4 commits March 19, 2018 17:42

Improved channel data parsing, error handling and readability

6205340

Optimized reconnect timeout and added docstrings

36adbd3

fixed reconnect test

b61ec01

Fixed logger call

2f5be5e

aoberoi approved these changes Mar 20, 2018

View reviewed changes

Roach merged commit 07cce9b into master Mar 20, 2018

Roach deleted the roach/rtm-reconnect branch March 20, 2018 23:53



		def test_rtm_reconnect_timeout_recently_connected(server, rtm_start_fixture):
		# If reconnected recently, server must wait to reconnect and increment the counter

Conversation

Roach commented Mar 3, 2018

Summary

Requirements (place an x in each [ ])

Uh oh!

codecov bot commented Mar 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Roach Mar 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Requirements (place an `x` in each `[ ]`)

codecov bot commented Mar 3, 2018 •

edited

Loading

Roach Mar 14, 2018 •

edited

Loading