PythonParser vs HiredisParser on_disconnect behavior

[popravich]

PythonParser's on_disconnect implementation is inconsistent with HiredisParser implementation (or vice versa):

class PythonParser(...):
    def on_disconnect(self):
        "Called when the socket disconnects"
        if self._sock is not None:
            self._sock.close()
            self._sock = None
        if self._buffer is not None:
            self._buffer.close()
            self._buffer = None
        self.encoder = None

and

class HiredisParser(...):
    def on_disconnect(self):
        self._sock = None
        self._reader = None
        self._next_response = False

Why does the PythonParser closes the _sock object?
By doing this the subsequent shutdown() and close() in Connection.disconnect does not make any sense, in fact it shutdown on closed socket raises error which is ignored.

I can submit a PR but please tell me what place to fix? (HiredisParser/PythonParser/shutdown)

PS: this issue causes other issues in other repos (celery/kombu#954, celery/celery#3898)

[theodesp]

I suspect it's because it's not covered with test cases. Feel free to provide a PR with coverage that does not break compatibility.

[popravich]

I'm sorry, but I don't understand, what a PR should cover?
Just add a test to show that PythonParser closes the Connection's socket and HiredisParser does not?

[andymccurdy]

You're right, the inconsistency is an error. From briefly scanning this, it seems wrong that PythonParser is closing the socket in on_disconnect.

Are you proposing that we simply remove the self._sock.close() from PythonParser.on_disconnect?

[popravich]

Are you proposing that we simply remove the self._sock.close() from PythonParser.on_disconnect?

Yes, this, at least, will make "on disconnect" behaviour similar regardless of parser used in connection.

[andymccurdy]

@popravich I agree. If you want to create a PR that removes the socket.close() I'll gladly merge it.

I'm also open to addressing the fork issues you're seeing in kombu/celery. Just let me know.

[popravich]

Hi, @andymccurdy,
I've just updated related PR with the fix, please take a look.