Regression: Connection fails re-opening connections in 1.0.0?

[gorlins]

I believe that ConnectionPool doesn't play nice with 1.0, and can yield broken connections even in the best case scenarios. Here's my reproducing code:

import happybase
from happybase.pool import ConnectionPool, TException, socket
import thriftpy

for mod in [happybase, thriftpy]:
    print mod.__name__, mod.__version__

kwargs = load_connection_kwargs()
print kwargs
pool = ConnectionPool(1, **kwargs)

for i in range(10):
    print 'Loop', i,
    try:
        with pool.connection() as conn:
            print 'opened',  # Ensure yield succeeds
            conn.tables()
            conn.close()  # Or any random failure/timeout etc
    except (TException, socket.error) as e:
        print 'failed', e
    else:
        print 'succeeded'

Works for 0.9:

happybase 0.9
thriftpy 0.3.8
{u'host': 'XXXX', 'protocol': 'binary', 'transport': 'framed'}
Loop 0 opened succeeded
Loop 1 opened succeeded
Loop 2 opened succeeded
Loop 3 opened succeeded
Loop 4 opened succeeded
Loop 5 opened succeeded
Loop 6 opened succeeded
Loop 7 opened succeeded
Loop 8 opened succeeded
Loop 9 opened succeeded

But not 1.0.0:

happybase 1.0.0
thriftpy 0.3.8
{u'host': 'XXXX', 'protocol': 'binary', 'transport': 'framed'}
Loop 0 opened succeeded
Loop 1 opened failed [Errno 9] Bad file descriptor
Loop 2 opened succeeded
Loop 3 opened failed [Errno 9] Bad file descriptor
Loop 4 opened succeeded
Loop 5 opened failed [Errno 9] Bad file descriptor
Loop 6 opened succeeded
Loop 7 opened failed [Errno 9] Bad file descriptor
Loop 8 opened succeeded
Loop 9 opened failed [Errno 9] Bad file descriptor

Given that ConnectionPool.connection() calls open() before yielding in both versions, I can't immediately see what causes this. I am using CDH 5.7 w/ hbase 1.2.0-cdh5.7.1.

[wbolster]

could be different behaviour in other python lib: thriftpy vs thrift (different dependencies in those versions).

also, stack trace gives more info perhaps?

[gorlins]

So I can actually simplify the error, happybase just cannot cleanly close and re-open a connection:

conn = Connection(**kwargs)
conn.open()
print 'First', conn.tables()  # Succeeds
conn.close()
conn.open()
print 'Second', conn.tables()  # Fails

Fails on the second call. Stack trace is the same for both

Traceback (most recent call last):
  File "junk.py", line 503, in <module>
    conn.tables()
  File "/Users/sgorlin/miniconda/envs/dna-f7cfb5b8/lib/python2.7/site-packages/happybase/connection.py", line 244, in tables
    names = self.client.getTableNames()
  File "/Users/sgorlin/miniconda/envs/dna-f7cfb5b8/lib/python2.7/site-packages/thriftpy/thrift.py", line 157, in _req
    self._send(_api, **kwargs)
  File "/Users/sgorlin/miniconda/envs/dna-f7cfb5b8/lib/python2.7/site-packages/thriftpy/thrift.py", line 169, in _send
    self._oprot.trans.flush()
  File "thriftpy/transport/framed/cyframed.pyx", line 107, in thriftpy.transport.framed.cyframed.TCyFramedTransport.flush (thriftpy/transport/framed/cyframed.c:2310)
  File "thriftpy/transport/framed/cyframed.pyx", line 95, in thriftpy.transport.framed.cyframed.TCyFramedTransport.c_flush (thriftpy/transport/framed/cyframed.c:2044)
  File "/Users/sgorlin/miniconda/envs/dna-f7cfb5b8/lib/python2.7/site-packages/thriftpy/transport/socket.py", line 129, in write
    self.sock.sendall(buff)
  File "/Users/sgorlin/miniconda/envs/dna-f7cfb5b8/lib/python2.7/socket.py", line 228, in meth
    return getattr(self._sock,name)(*args)
  File "/Users/sgorlin/miniconda/envs/dna-f7cfb5b8/lib/python2.7/socket.py", line 174, in _dummy
    raise error(EBADF, 'Bad file descriptor')
socket.error: [Errno 9] Bad file descriptor

[gorlins]

For context I'll add my justifying use case. I get frequent timeouts in the form:

conn = Connection(**kwargs)
conn.tables()  # works
from time import sleep; sleep(75)
conn.tables()
TTransportException: TTransportException(message='TSocket read 0 bytes', type=4)

So I use a subclass of ConnectionPool which tries to shield the consuming code from bad connections:

class AutoRetryPool(ConnectionPool):
    """Connection pool which does its best to ensure open connections

    May impose some overhead (ie not good for low-latency stuff) but should
    increase reliability of getting a working, open connection when needed

    """
    def __init__(self, size, retries=5, **kwargs):
        self.retries = retries
        super(AutoRetryPool, self).__init__(size, **kwargs)

    @contextmanager
    def connection(self, timeout=None):
        attempts = 0
        opened = False
        while True:
            try:
                with super(AutoRetryPool, self).connection(timeout=timeout) as conn:
                    # Force refresh the client
                    conn.close()
                    conn.open()

                    # Make an actual call to ensure
                    _ = conn.tables()
                    opened = True
                    yield conn
                return

            except (TException, socket.error):
                # These trigger refreshed connections in parent
                if opened:
                    # aka the failure was inside the yield
                    raise
                attempts += 1
                # print 'Replacing connection, attempt', attempts
                if attempts >= self.retries:
                    raise

I can't simply let ConnectionPool refresh the connection because it could idle in the queue long enough before it is requested again (aside: perhaps open() should not be called in the except block in ConnectionPool.connection(), so it only opens lazily when the conn is up). Perhaps the right code to use entails Connection._refresh_thrift_client but as a private method, this is unsatisfactory - the public interfaces of Connection.open() and Connection.close() should be the only methods used by other classes to refresh the client and it's not clear to me how that method relates.

[wbolster]

since Connection.open() only calls transport.open(),at first glance it seems the problem is in the thriftpy lib. can you try opening a bug report there (and refer to this one)?

https://github.com/wbolster/happybase/blob/master/happybase/connection.py#L169-L178

[gorlins]

I'm not sure I understand the underlying bug/difference between thriftpy and thrift, but more importantly, based on my understanding of the code, I think this should be addressable by Connection and not upstream.

Expected behavior of conn.close(); conn.open(), to me, is that the thing return to a clean working state. What is the purpose of conn._refresh_thrift_client if it is not envoked in that sequence? Maybe:

def open(self):
    if self.transport is None:
        self._refresh_thrift_client()
    # rest of method omitted

def close(self):
    if self.transport is None or not self.transport.is_open():
        self.transport = None
        return

    # logging omitted
    self.transport.close()
    self.transport = None  # Release all socket-like resources for gc
    self.client = None  # not sure about this one

Unless you need persistent transport/client/etc objects for some reason? I know that conn.client use leaks out in Table but ideally these should be totally encapsulated inside Connection

[gorlins]

cause identified in other ticket, relying on self.transport.is_open() is unsafe in this instance.

[gorlins]

thriftpy is bumped to 0.3.9, do you want to up requirements and release 1.0.1? I'll consider a pull request for the other issues

[wbolster]

does happybase need changes? or is just bumping the thriftpy dep enough?

[gorlins]

i think the dep is enough here, if i have time i'll try a pull request to make more robust but the acute issue is resolved by thriftpy

[aswinQuiklo]

I still get the TTransportException(message='TSocket read 0 bytes', type=4) exception if I keep my connection alive for some ~2 minutes. I tried creating connection by specifying timeout=0, but that doesn't seem to help. Can you guys tell me what to do if I have to keep a connection alive forever and run queries on HBase? I am using HBase version 1.2.1

[gorlins]

Here is what I currently have deployed, although I have not recently verified whether the extra work is still necessary

class AutoRetryPool(ConnectionPool):
    """Connection pool which does its best to ensure open connections

    May impose some overhead (ie not good for low-latency stuff) but should
    increase reliability of getting a working, open connection when needed

    """
    def __init__(self, size, retries=5, **kwargs):
        self.retries = retries
        super(AutoRetryPool, self).__init__(size, **kwargs)

    @contextmanager
    def connection(self, timeout=None):
        attempts = 0
        opened = False
        while True:
            try:
                with super(AutoRetryPool, self).connection(timeout=timeout) as conn:
                    # Make an actual call to ensure
                    _ = conn.tables()
                    opened = True
                    yield conn
                return

            except (TException, socket.error):
                # These trigger refreshed connections in parent
                if opened:
                    # aka the failure was inside the yield, can't fix
                    raise

                attempts += 1
                # print 'Replacing connection, attempt', attempts
                if attempts >= self.retries:
                    raise

    def _return_connection(self, connection):
        # Don't leave idling connections around - too easy to timeout before use
        connection.close()
        return super(AutoRetryPool, self)._return_connection(connection)

[aswinQuiklo]

Thanks! I will test this and get back to you!