bpo-30985: set closing variable in asyncore #2804

beltran · 2017-07-21T16:07:50Z

Could you take a look at this @nirs ?

nirs

Looks good, but need to check why the windows CI is failing.

nirs · 2017-07-21T17:51:18Z

Lib/test/test_asyncore.py

@@ -651,6 +651,7 @@ def writable(self):
        server = BaseServer(self.family, self.addr, TestHandler)
        client = TestClient(self.family, server.address)
        self.loop_waiting_for_flag(client)
+        self.assertTrue(client.closing)


I think a separate simpler test for closing once and twiice is better, similar to the file_wrapper tests. Here we are mixing 2 issues in the same test. The purpose of this test is handling of ECONNRESET/EPIPE, while the closing test is about 1. setting closing on close, and 2. handling double close gracefully.

nirs · 2017-07-21T17:53:00Z

Lib/asyncore.py

@@ -388,6 +388,10 @@ def recv(self, buffer_size):
                raise

    def close(self):
+        if self.closing:
+            return
+        self.closing = True


IMHO this change should be documented in asyncore documentation using a ".. versionchanged:: 3.7" markup.

nirs

Looks good to me, @Haypo, do you want to take a look?

giampaolo

LGTM

giampaolo · 2017-07-25T21:47:10Z

Lib/test/test_asyncore.py

+    def test_close(self):
+        d = asyncore.dispatcher()
+        d.close()
+        self.assertTrue(d.closing)


Just a sec: I would check d.closing is False before close().

nirs

This will break user code adding closing-once to their dispatcher subclass using dispatcher.closing:

def close(self):
    if self.closing:
        return
    self.closing = True
    asyncore.dispatcher.close(self)

I expect this to be very common. Using the undocumented closing attribute is wrong, but we cannot do this.

I suggest to use the private _fileno to detect closing, see file_wrapper.close().

giampaolo · 2017-07-26T09:55:08Z

This will break user code adding closing-once to their dispatcher subclass using dispatcher.closing:

Uh? I don't understand how it would break code.

Also, I suggest to call this "__closed" instead, and add a comment telling all new attributes added in the future should have a "__" prefix, see discussion at:
http://bugs.python.org/issue30985

nirs · 2017-07-26T10:09:23Z

@giampaolo, see the code example in my comment - if the subclass set closing to True befor calling the superclass close(), superclass will not nothing :-)

giampaolo · 2017-07-26T10:11:35Z

Ah sorry, I get what you mean now. Yes, there is no way to be 100% sure names won't clash, but with __closed the chances should be way lower.

nirs · 2017-07-26T10:47:05Z

@giampaolo, @Haypo, with #2854, I don't think we need to detect if a dispatcher was closed.

The dispatcher is already protected in python 3.7 in multiple ways:

del_channel is using self._fileno
close() catch EBADF from self.socket.close()
socket.close() does not raise EBADF when calling multiple times (tested only on Linux)

So we don't need any change :-)

If we want to have cleaner close-once semantics, we can add 2 lines:

    def close(self):
        if self._fileno is None:
            return

giampaolo · 2017-07-26T12:18:47Z

+1 for relying on _fileno

vstinner · 2017-07-26T12:22:48Z

IMHO https://bugs.python.org/issue30931 is now well defined: the use case is described, the constraints have been listed, different implementations have been discussed, etc.

I'm sorry, but I don't get the use case for http://bugs.python.org/issue30985 since internally, asyncore doesn't use closing!? It was said multiple times that there is a high risk of regression. Ok. What about the benefits?

The only mentionned use case is to fix a race condition, but as I wrote at http://bugs.python.org/issue30985#msg299165 , the closing attribute is not needed to fix the race condition.

Note: We usually prefer to discuss issues at bugs.python.org.

giampaolo · 2017-07-26T12:26:13Z

By relying on _fileno there's no risk of breaking compatibility. The check should go in regardless of https://bugs.python.org/issue30931 because a double close() can still occur on read() followed write().

nirs · 2017-07-26T13:57:45Z

With #2854, I don't think we can have write() after read() if read() has closed the dispatcher.

We can still have this readwrite():

def readwrite(obj, flags):  
    try:
        if flags & select.POLLIN:
            obj.handle_read_event()
        if flags & select.POLLOUT:
            obj.handle_write_event()
        if flags & select.POLLPRI:
            obj.handle_expt_event()
        if flags & (select.POLLHUP | select.POLLERR | select.POLLNVAL):
            obj.handle_close()
    ...

vstinner · 2017-07-26T16:02:12Z

@giampaolo: "By relying on _fileno there's no risk of breaking compatibility."

I'm sorry, but I'm lost. There are many PR related to asyncore and many things were discussed. Why do you want to use _fileno? What is your use case? Are you trying to fix a bug?

Can we please wait until https://bugs.python.org/issue30931 is fixed? It would clarify the discussion.

vstinner · 2017-07-26T16:05:51Z

The check should go in regardless of https://bugs.python.org/issue30931 because a double close() can still occur on read() followed write().

Hum. I guess that you describe a bug when poll2() is used, a FD is both ready to read and ready to write, the read handler closes the FD, and then the write handler tries to use the closed FD?

Why using using the same pattern used in poll() of the PR 2854, check "map.get(fd) is not obj" before calling each handler? For example, inline readwrite() in poll2() and duplicate the check?

nirs · 2017-07-26T16:10:00Z

Or we can add a check in handle_xxx_event for closed channel (self._fileno is None), and do nothing in this case, or raise, and handle the error in readwrite.

giampaolo · 2017-07-26T16:20:45Z

@giampaolo: "By relying on _fileno there's no risk of breaking compatibility."
I'm sorry, but I'm lost. There are many PR related to asyncore and many things were discussed. Why do > you want to use _fileno? What is your use case? Are you trying to fix a bug?

Yes, I want to prevent close to be called twice during a single IO loop.
That can happen if a fd is both readable and writable, it gets closed on read(), then it gets closed again on write().

vstinner · 2017-07-26T16:23:23Z

I added another fix to the poll2() race condition described by @giampaolo in my PR 2854, see e701582

giampaolo · 2017-07-26T16:26:06Z

Or we can add a check in handle_xxx_event for closed channel (self._fileno is None), and do nothing in this case, or raise, and handle the error in readwrite.

I would be scared to do that. Any change to asyncore, even minimal, should be very cautious. It's API is so broken that basically all subclasses out there rely on the long established logic of its internals, including logic which is clearly broken, like calling handle_*_event methods even when a dispatcher has already been closed.

vstinner · 2017-07-26T16:27:41Z

"Yes, I want to prevent close to be called twice during a single IO loop. That can happen if a fd is both readable and writable, it gets closed on read(), then it gets closed again on write()."

Ok, you are right that my PR didn't prevent this bug, but only fixed two other race conditions. But I added a third fix in my PR ;-)

What matters for me is not really "prevent close to be called twice" but more generally the semantics. If I understood correctly, the constraint is:

asyncore must not call a handler if its dispatcher was closed

I believe that my PR now correctly implements this constraint in all cases. I wrote 3 different unit tests to test the 3 described bugs.

vstinner · 2017-07-26T16:30:21Z

@nirs: "Or we can add a check in handle_xxx_event for closed channel (self._fileno is None), and do nothing in this case, or raise, and handle the error in readwrite."

I'm not sure that I understand your suggestion. handlexx_event() are written in third party code. Do you suggest to modify all projects on PyPI and private projects?

IMHO it's simpler to do the check just before calling an handler, no?

giampaolo · 2017-07-26T16:34:03Z

For now I would start focusing on this PR and use _fileno instead of closing, merge it, backport it and then start looking at the other PRs.

vstinner · 2017-07-26T16:50:44Z

@giampaolo: "I would be scared to do that. Any change to asyncore, even minimal, should be very cautious. It's API is so broken that basically all subclasses out there relies on the long established logic of its internals, including logic which is clearly broken, like calling handle_*_event methods even when a dispatcher has already been closed."

Same for me here, I'm very scared :-) But my PR #2854 clearly changed what you call a "clearly broken logic". Are you saying that my change is backward incompatible?

Honestly, if someone relies on the broken behaviour, I would suggest to copy the short asyncore.py file of an older Python version to really make sure that the behaviour never changes.

We cannot fix the 3 discussed race conditions without changing the exact semantics.

I would add that another issue is that the semantics is not currently well defined. For example, there was no test to check if read is called before write. My PollTests test case adds such "semantics" tests.

nirs · 2017-07-26T17:02:43Z

@Haypo it is harder to check before calling the handler, because of the way handlers call other handlers internally, see handle_read_event().

Regarding fixing hadnle_xxx_event() - they are undocumented, private implemetation defail of asyncore. Users of this class should implement only handle_xxx() method, specified in the docs. I don't know if people reimplemented them.

I think #2854 first commit is fine and can be merged without the new commit, and if we want to protect from double close we can do 2 line change in close().

Calling asyncore handle_xxx() after it was closed is an old issue. Here is one bug I reported 8 years ago:
http://bugs.python.org/issue6550

vstinner · 2017-07-26T17:07:44Z

Nir:

Calling asyncore handle_xxx() after it was closed is an old issue. Here is one bug I reported 8 years ago: http://bugs.python.org/issue6550

Oh. I closed this one. I still agree with what I wrote in 2014 :-)

asyncore-fix-refused-4.patch catchs EBADF. To me, it looks like a bug: if you get a EBADF error, we already made something wrong before.

giampaolo · 2017-07-26T17:09:04Z

Regarding fixing hadnle_xxx_event() - they are undocumented, private implemetation defail of asyncore. I don't know if people reimplemented them.

Unfortunately I know at least 2 code bases relying on such methods, one is pyftpdlib, another one is at my previous company (smartfile). This is exactly what I was talking about. In asyncore/chat everything is "public", even the non-documented stuff.

vstinner · 2017-07-26T17:12:57Z

Maybe it will be easier if we agree that some asyncore cannot be fixed by design ;-)

taleinat · 2018-09-06T05:18:54Z

@vstinner, @giampaolo, should this be closed?

vstinner · 2018-09-06T08:33:02Z

Sadly, I agree to leave asyncore unchanged and so reject this PR. It's deprecated since Python 3.6. Just upgrade to asyncio, threads or something else. Backward compatibility matters more here.

giampaolo · 2018-09-06T09:48:35Z

I agree with @vstinner. This should be rejected because basically any change to asyncore poses a backward compatibility risk due to its poor API.

the-knights-who-say-ni added the CLA signed label Jul 21, 2017

nirs reviewed Jul 21, 2017

View reviewed changes

bjmb added 2 commits July 21, 2017 15:52

Checked for closing being set to true after socket closed in asyncore

4f0ea7c

Set closing to True in close in asyncore

673904c

beltran force-pushed the closing_asyncore branch from 5610268 to 673904c Compare July 21, 2017 19:52

nirs approved these changes Jul 24, 2017

View reviewed changes

nirs mentioned this pull request Jul 24, 2017

bpo-30931: Fix asyncore race condition #2854

Closed

giampaolo approved these changes Jul 25, 2017

View reviewed changes

giampaolo self-assigned this Jul 25, 2017

giampaolo reviewed Jul 25, 2017

View reviewed changes

nirs suggested changes Jul 26, 2017

View reviewed changes

brettcannon added the awaiting merge label Feb 2, 2018

vstinner closed this Sep 6, 2018

Uh oh!

bpo-30985: set closing variable in asyncore #2804

bpo-30985: set closing variable in asyncore #2804

Uh oh!

Conversation

beltran commented Jul 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nirs left a comment

Choose a reason for hiding this comment

Uh oh!

nirs Jul 21, 2017

Choose a reason for hiding this comment

Uh oh!

nirs Jul 21, 2017

Choose a reason for hiding this comment

Uh oh!

vstinner Jul 25, 2017

Choose a reason for hiding this comment

Uh oh!

nirs left a comment

Choose a reason for hiding this comment

Uh oh!

giampaolo left a comment

Choose a reason for hiding this comment

Uh oh!

giampaolo Jul 25, 2017

Choose a reason for hiding this comment

Uh oh!

nirs left a comment

Choose a reason for hiding this comment

Uh oh!

giampaolo commented Jul 26, 2017

Uh oh!

nirs commented Jul 26, 2017

Uh oh!

giampaolo commented Jul 26, 2017

Uh oh!

nirs commented Jul 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

giampaolo commented Jul 26, 2017

Uh oh!

vstinner commented Jul 26, 2017

Uh oh!

giampaolo commented Jul 26, 2017

Uh oh!

nirs commented Jul 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vstinner commented Jul 26, 2017

Uh oh!

vstinner commented Jul 26, 2017

Uh oh!

nirs commented Jul 26, 2017

Uh oh!

giampaolo commented Jul 26, 2017

Uh oh!

vstinner commented Jul 26, 2017

Uh oh!

giampaolo commented Jul 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vstinner commented Jul 26, 2017

Uh oh!

vstinner commented Jul 26, 2017

Uh oh!

giampaolo commented Jul 26, 2017

Uh oh!

vstinner commented Jul 26, 2017

Uh oh!

nirs commented Jul 26, 2017

Uh oh!

vstinner commented Jul 26, 2017

Uh oh!

giampaolo commented Jul 26, 2017

Uh oh!

vstinner commented Jul 26, 2017 via email

Uh oh!

taleinat commented Sep 6, 2018

beltran commented Jul 21, 2017 •

edited

Loading

nirs commented Jul 26, 2017 •

edited

Loading

nirs commented Jul 26, 2017 •

edited

Loading

giampaolo commented Jul 26, 2017 •

edited

Loading