cryptofeed icon indicating copy to clipboard operation
cryptofeed copied to clipboard

WSAsyncConn is_open implementation

Open anovv opened this issue 3 years ago • 1 comments

I noticed that current implementation of is_open in WSAsyncConn looks a bit buggy:

@property
def is_open(self) -> bool:
    return self.conn and not self.conn.closed

self.conn is an instance of websockets.WebSocketClientProtocol, which in fact has 4 states: CONNECTING, OPEN, CLOSING and CLOSED, which means that not self.conn.closed does not imply that connection is actually open.

This may lead to inconsistencies, e.g. in my fork I have a watcher which checks connection health by looking at is_open property. If I specify wrong ws address or if ws address is unable to connect and indefinitely timeouts and retries, the connection is still marked as open as is_open returns True.

Is there a reason for this implementation? Should we change not self.conn.closed to self.conn.open? Or at least rename is_open to not_closed whenever it is used to not break current implementation (for whatever reason it was introduced) and make is_open return proper state?

anovv avatar May 10 '22 08:05 anovv

this is from the docs:

property open: bool True when the connection is open; False otherwise.

This attribute may be used to detect disconnections. However, this approach is discouraged per the EAFP principle. Instead, you should handle ConnectionClosed exceptions.

property closed: bool True when the connection is closed; False otherwise.

Be aware that both open and closed are False during the opening and closing sequences.

The last part seems concerning, seems like we shouldnt use these attributes at all for these things, but that the exception should be handled. that being said, if you have a PR in mind that solves the issue, I'll review it

bmoscon avatar May 22 '22 18:05 bmoscon