Fix socket leaks #351

it-x · 2017-02-27T21:11:49Z

The close method moved to finally block, so closing a socket will occur even in case of exception

the-knights-who-say-ni · 2017-02-27T21:11:51Z

Hello, and thanks for your contribution!

I'm a bot set up to make sure that the project can legally accept your contribution by verifying you have signed the PSF contributor agreement (CLA).

Unfortunately we couldn't find an account corresponding to your GitHub username on bugs.python.org (b.p.o) to verify you have signed the CLA. This is necessary for legal reasons before we can look at your contribution. Please follow these steps to help rectify the issue:

If you don't have an account on b.p.o, please create one
Make sure your GitHub username is listed in "Your Details" at b.p.o
If you have not already done so, please sign the PSF contributor agreement. The "bugs.python.org username " requested by the form is the "Login name" field under "Your Details".
If you just signed the CLA, please wait at least one US business day and then check "Your Details" on bugs.python.org to see if your account has been marked as having signed the CLA (the delay is due to a person having to manually check your signed CLA)
Reply here saying you have completed the above steps

Thanks again to your contribution and we look forward to looking at it!

it-x · 2017-02-28T19:06:31Z

I'm signed the CLA

vstinner

"with conn:" cannot be used?

vstinner · 2017-03-02T10:44:10Z

Oh, ignore my comment, I missed the Python version (2.7): in Python 2.7, socket.socket doesn't support the context manager protocol :-(

vstinner

retrbinary(), retrlines() and storelines() at least have the same bug, no? Can you please fix similar bugs in other functions?

it-x · 2017-03-02T17:43:23Z

Ok, I did

Mariatta · 2017-03-04T15:07:28Z

Should there be an issue in the bug tracker for this?

Also, I'm applying the cherry-pick for 2.7 label here to make it more obvious that this is for 2.7 branch :)

serhiy-storchaka · 2017-04-02T13:23:17Z

Lib/ftplib.py

+                callback(line)
+        finally:
+            if fp:
+                fp.close()


Can fp.close() fail? If yes, conn will be not closed.

The file is in read mode, I don't see why fp.close() would fail.

An alternative is to use two try/finally: conn and then fp. It would avoid "fp = None" as well. Do you think that it's worth it @serhiy-storchaka ?

KeyboardInterrupt ;-)

serhiy-storchaka · 2017-04-02T13:23:49Z

You can use contextlib.closing().

vstinner

LGTM, but I prefer to wait for @serhiy-storchaka for retrlines().

vstinner · 2017-04-03T09:54:12Z

Lib/ftplib.py

+                callback(line)
+        finally:
+            if fp:
+                fp.close()


An alternative is to use two try/finally: conn and then fp. It would avoid "fp = None" as well. Do you think that it's worth it @serhiy-storchaka ?

serhiy-storchaka

LGTM. I think we can ignore very small issue in retrlines() in 2.7. I just remembered of alternatives for the case if you missed them.

vstinner · 2017-04-03T11:06:57Z

Thank you @J0l-lnny for your fix! I merged your PR.

vstinner · 2017-04-03T11:14:19Z

@serhiy-storchaka: "LGTM. I think we can ignore very small issue in retrlines() in 2.7."

Right, thank you for your review. If someone wants to handle the most complex corner cases, I suggest to focus on the master branch. Right now, the master branch uses a "nested" with which isn't ideal since it doesn't handle exceptions in creating fp fails (or is interrupted by CTRL+c or whatever):

Extract of ftplib.py in the master branch:

    def retrlines(self, cmd, callback = None):
        ...
        with self.transfercmd(cmd) as conn, \
                 conn.makefile('r', encoding=self.encoding) as fp:
        ...

At least, it ensures that conn and fp are closed properly, right?

serhiy-storchaka · 2017-04-03T15:52:06Z

What is wrong with a nested with? The resources management in the master branch looks correct to me.

vstinner · 2017-04-03T15:58:46Z

What is wrong with a nested with? The resources management in the master branch looks correct to me.

If makefile() raises an exception, conn is not closed. Well, in fact I'm not sure, that's why I was asking :-)

serhiy-storchaka · 2017-04-03T16:06:56Z

It is properly closed.

with a, b:
        ...

is just a syntax sugar for

with a:
    with b:
        ...

You perhaps missed with deprecated and removed

with contextlib.nested(a, b):
        ...

vstinner · 2017-04-03T16:17:39Z

(...) is just a syntax sugar for "with a: with b: ..."

Ok! master is fine in that case ;-) Thanks for the confirmation. Note: By "nested", I really mean "with expr1 as a, expr2 as b: ...".

Bumps [celery](https://github.com/celery/celery) from 4.4.2 to 4.4.6. - [Release notes](https://github.com/celery/celery/releases) - [Changelog](https://github.com/celery/celery/blob/master/Changelog.rst) - [Commits](celery/celery@4.4.2...v4.4.6) Signed-off-by: dependabot-preview[bot] <[email protected]> Co-authored-by: dependabot-preview[bot] <27856297+dependabot-preview[bot]@users.noreply.github.com>

Fix socket leaks

bb98ca3

the-knights-who-say-ni added the CLA not signed label Feb 27, 2017

brettcannon removed the CLA not signed label Feb 28, 2017

the-knights-who-say-ni added the CLA signed label Feb 28, 2017

vstinner reviewed Mar 2, 2017

View reviewed changes

vstinner requested changes Mar 2, 2017

View reviewed changes

Fixed sockets leak

9ae87b8

vstinner approved these changes Mar 2, 2017

View reviewed changes

Mariatta added the cherry-pick for 2.7 label Mar 4, 2017

serhiy-storchaka reviewed Apr 2, 2017

View reviewed changes

vstinner reviewed Apr 3, 2017

View reviewed changes

serhiy-storchaka approved these changes Apr 3, 2017

View reviewed changes

vstinner merged commit d64146c into python:2.7 Apr 3, 2017

it-x deleted the 2.7 branch April 7, 2017 18:22

Uh oh!

Fix socket leaks #351

Fix socket leaks #351

Uh oh!

Conversation

it-x commented Feb 27, 2017

Uh oh!

the-knights-who-say-ni commented Feb 27, 2017

Uh oh!

it-x commented Feb 28, 2017

Uh oh!

vstinner left a comment

Choose a reason for hiding this comment

Uh oh!

vstinner commented Mar 2, 2017

Uh oh!

vstinner left a comment

Choose a reason for hiding this comment

Uh oh!

it-x commented Mar 2, 2017

Uh oh!

Mariatta commented Mar 4, 2017

Uh oh!

serhiy-storchaka Apr 2, 2017

Choose a reason for hiding this comment

Uh oh!

vstinner Apr 3, 2017

Choose a reason for hiding this comment

Uh oh!

vstinner Apr 3, 2017

Choose a reason for hiding this comment

Uh oh!

serhiy-storchaka Apr 3, 2017

Choose a reason for hiding this comment

Uh oh!

serhiy-storchaka commented Apr 2, 2017

Uh oh!

vstinner left a comment

Choose a reason for hiding this comment

Uh oh!

vstinner Apr 3, 2017

Choose a reason for hiding this comment

Uh oh!

serhiy-storchaka left a comment

Choose a reason for hiding this comment

Uh oh!

vstinner commented Apr 3, 2017

Uh oh!

vstinner commented Apr 3, 2017

Uh oh!

serhiy-storchaka commented Apr 3, 2017

Uh oh!

vstinner commented Apr 3, 2017

Uh oh!

serhiy-storchaka commented Apr 3, 2017

Uh oh!

vstinner commented Apr 3, 2017 via email

Uh oh!

Uh oh!