Fix regression in v2.3.0

[fatih-acar]

Make sure we do not close the running session

We must only close the temporary session used for the request.

Fixes: 854920d

Signed-off-by: Fatih Acar f.acar@criteo.com

Summary

Latest release (v2.3.0) introduced a regression which make our slack app crash.
The associated traceback looks like:

Traceback (most recent call last):
  File "/home/f.acar/hwinfo-slackbot/venv/lib64/python3.6/site-packages/slack/rtm/client.py", line 334, in _connect_and_read
    proxy=self.proxy,
  File "/home/f.acar/hwinfo-slackbot/venv/lib64/python3.6/site-packages/aiohttp/client.py", line 1012, in __aenter__
    self._resp = await self._coro
  File "/home/f.acar/hwinfo-slackbot/venv/lib64/python3.6/site-packages/aiohttp/client.py", line 728, in _ws_connect
    proxy_headers=proxy_headers)
  File "/home/f.acar/hwinfo-slackbot/venv/lib64/python3.6/site-packages/aiohttp/client.py", line 357, in _request
    raise RuntimeError('Session is closed')
RuntimeError: Session is closed

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.

[CLAassistant]

All committers have signed the CLA.

[codecov]

Codecov Report

Merging #544 into master will decrease coverage by 0.02%.
The diff coverage is 50%.

@@            Coverage Diff             @@
##           master     #544      +/-   ##
==========================================
- Coverage   68.87%   68.85%   -0.03%     
==========================================
  Files          15       15              
  Lines        1722     1724       +2     
  Branches       97       98       +1     
==========================================
+ Hits         1186     1187       +1     
  Misses        511      511              
- Partials       25       26       +1

Impacted Files	Coverage Δ
slack/web/base_client.py	79.64% <50%> (-0.54%)	⬇️

---

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 a7271b0...4f167d0. Read the comment docs.

[seratch]

If I understand correctly, does the code you intended seem to be 2) in the following examples?

class Session:
  def closed(self):
    true

null_session = None
closed_session = Session()

# 1)
if not null_session or closed_session.closed:
  print("x")

# 2)
if not (null_session or closed_session.closed):
  print("y")

Result:

>>> class Session:
...   def closed(self):
...     true
... 
>>> null_session = None
>>> closed_session = Session()
>>> 
>>> 
>>> if not null_session or closed_session.closed:
...   print("x")
... 
x
>>> 
>>> if not (null_session or closed_session.closed):
...   print("y")
... 
>>> 

[fatih-acar]

No it's not, it's rather if not (null_session and not closed_session.closed): which is the negation of the condition at this line https://github.com/slackapi/python-slackclient/blob/master/slack/web/base_client.py#L249.
The idea is to close the session created here: https://github.com/slackapi/python-slackclient/blob/master/slack/web/base_client.py#L252.

I updated the PR accordingly to avoid this confusion.

[seratch]

Thanks for the further explanation. I understand the point.

To me, double negative logic looks a bit confusing. Defining a local value may be easier-to-understand (and is good for maintainability, too).

use_running_session = self.session and not self.session.closed
if use_running_session:
    session = self.session
else:
    session = aiohttp.ClientSession(
        timeout=aiohttp.ClientTimeout(total=self.timeout),
        auth=req_args.pop("auth", None),
    )

(omitted)

if not use_running_session:
  await session.close()

[fatih-acar]

You're right, this approach looks better.
Thanks for your input, I've updated the commit.

[seratch]

@fatih-acar Thanks for the contribution! A new version will be shipped very soon.

[amilbourne]

Thanks for fixing this. Do you have any idea when it will be released?
The reason I ask is that I was hit by bug #535 when using version 2.2.1 so upgraded to 2.3.0 and was immediately hit by this issue. I may be able to downgrade but was hoping to avoid that.

[RodneyU215]

I'll be releasing a patch today.

[amilbourne]

Thanks for fixing this.