earl / beanstalkc

A simple beanstalkd client library for Python
Apache License 2.0
457 stars 115 forks source link

socket close bug? #34

Closed yancl closed 10 years ago

yancl commented 11 years ago

Hi, I notice there may be an socket close bug after viewing the beanstalkc code:

    def close(self):
        """Close connection to server."""
        try:
            self._socket.sendall('quit\r\n')
            self._socket.close()
        except socket.error:
            pass

    def reconnect(self):
        """Re-connect to server."""
        self.close()
        self.connect() 

suppose i made a connection to beanstalkd, but the server close the socket for some reason(server exit,for example). then beanstalkc raise an exception of socket error. so the client code make a reconnect call to establish a new connection to the server. but as you see in the close method, it first sendall('quit'). that will beak. so it does not work around in this case. mybe we could just close the socket,am i right? thanks:)

Doerge commented 11 years ago
def close(self):
    """Close connection to server."""
    try:
        self._socket.sendall('quit\r\n')
    except socket.error:
        pass
    self._socket.close()

I agree. Even though the socket is eventually closed when garbage-collected we might as well do it explicitly.

earl commented 11 years ago

Right, that's a bug. @Doerge's suggestion looks good. We should check if socket#close() can raise a socket.error as well, and if it does, also wrap it accordingly.

Doerge commented 11 years ago

Good point.

I think we should wrap it then, as close can raise socket.error in certain crazy situations. E.g. if you have already called close before, on interrupt-signals and some filesystem dependent stuff as well apparently: http://www.gnu.org/software/libc/manual/html_node/Opening-and-Closing-Files.html#Opening-and-Closing-Files I'm not exactly sure how Python handles those errors but I would expect them to be wrapped as socket.error so nothing else can be thrown from calling close(). However to make it bulletproof we could just do a generic try-except without specifying which exception to catch.

earl commented 10 years ago

Fixed by #35.