agoragames / haigha

AMQP Python client
BSD 3-Clause "New" or "Revised" License
160 stars 41 forks source link

Logic error in SocketTransport.read() #26

Closed vitaly-krugl closed 12 years ago

vitaly-krugl commented 12 years ago

In the code snippet below from SocketTransport.read(), there is an elif block "elif isinstance(e, socket.timeout):" inside the "except EnvironmentError as e:". It's not clear why that elif is inside "except EnvironmentError as e:" since socket.timeout has its own "except socket.timeout as e:" block:

  def read(self, timeout=None):
    '''
    Read from the transport. If timeout>0, will only block for `timeout` 
    seconds.
    '''
    if not hasattr(self,'_sock'):
      return None

    try:
      # Note that we ignore both None and 0, i.e. we either block with a
      # timeout or block completely and let gevent sort it out.
      if timeout:
        self._sock.settimeout( timeout )
      else:
        self._sock.settimeout( None )
      data = self._sock.recv( self._sock.getsockopt(socket.SOL_SOCKET,socket.SO_RCVBUF) )

      if len(data):
        if self.connection.debug > 1:
          self.connection.logger.debug( 'read %d bytes from %s'%(len(data), self._host) )
        if len(self._buffer):
          self._buffer.extend( data )
          data = self._buffer
          self._buffer = bytearray()
        return data

      # Note that no data means the socket is closed and we'll mark that
      # below

    except socket.timeout as e:
      # Note that this is implemented differently and though it would be
      # caught as an EnvironmentError, it has no errno. Not sure whose
      # fault that is.
      return None

    except EnvironmentError as e:
      # thrown if we have a timeout and no data
      if e.errno in (errno.EAGAIN,errno.EWOULDBLOCK):
        return None
      # gevent throws this too, and rather than handle separately just catch
      # that case here
      elif isinstance(e, socket.timeout):
        return None
awestendorf commented 12 years ago

Thanks for spotting that. I think it was leftover from when I added the base class and ported most of the gevent implementation into it.