[
https://issues.apache.org/jira/browse/THRIFT-5861?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Asjad updated THRIFT-5861:
--------------------------
Description:
Normally I include a check like this before closing Thrift connections:
{code:python}
if transport and transport.isOpen():
transport.close() {code}
However, for Tornado, {{TTornadoStreamTransport.isOpen()}} is not implemented,
so the call falls back to {{{}TTransportBase{}}}'s empty implementation which
just returns {{{}None{}}}. As a result, the above condition to close the
connection never gets satisfied, and my application accumulates open
connections until eventually it crashes with error message {{{}OSError: [Errno
24] Too many open files{}}}.
To fix this, this patch implements {{{}TTornadoStreamTransport.isOpen(){}}},
using Tornado's
[{{BaseIOStream.closed()}}|https://www.tornadoweb.org/en/stable/iostream.html#tornado.iostream.BaseIOStream.closed],
which returns {{True}} if the stream has been closed. Before checking if the
stream is open, we need to verify that the stream exists.
To test the patch, we can add these print statements into
[thrift/tutorial/py.tornado/PythonClient.py|https://github.com/apache/thrift/blob/2e00c9998f1aa316c8d0168488887fb957845230/tutorial/py.tornado/PythonClient.py#L46]
in the appropriate places:
{code:python}
print(f"transport.isOpen() before transport.open(): {transport.isOpen()}")
print(f"transport.isOpen() after transport.open(): {transport.isOpen()}")
print(f"transport.isOpen() after TTransportException: {transport.isOpen()}")
print(f"transport.isOpen() before transport.close(): {transport.isOpen()}")
print(f"transport.isOpen() after transport.close(): {transport.isOpen()}")
{code}
Without the patch applied, {{isOpen()}} always returns {{{}None{}}}, regardless
of whether the transport is opened or closed:
{noformat}
transport.isOpen() before transport.open(): None
transport.isOpen() after transport.open(): None
transport.isOpen() before transport.close(): None
transport.isOpen() after transport.close(): None{noformat}
{noformat}
transport.isOpen() before transport.open(): None
ERROR:root:could not connect to 127.0.0.1:9090 (Stream is closed)
transport.isOpen() after TTransportException: None{noformat}
With the patch applied, {{isOpen()}} accurately reflects the transport state:
{noformat}
transport.isOpen() before transport.open(): False
transport.isOpen() after transport.open(): True
transport.isOpen() before transport.close(): True
transport.isOpen() after transport.close(): False{noformat}
{noformat}
transport.isOpen() before transport.open(): False
ERROR:root:could not connect to localhost:9090 (Stream is closed)
transport.isOpen() after TTransportException: False{noformat}
It also worked neatly with reopening the transport but I didn't include that as
it didn't fit in neatly with the tutorial's outline.
> Add isOpen method to TTornadoStreamTransport
> --------------------------------------------
>
> Key: THRIFT-5861
> URL: https://issues.apache.org/jira/browse/THRIFT-5861
> Project: Thrift
> Issue Type: Improvement
> Components: Python - Library
> Affects Versions: 0.21.0
> Reporter: Asjad
> Priority: Minor
>
> Normally I include a check like this before closing Thrift connections:
> {code:python}
> if transport and transport.isOpen():
> transport.close() {code}
> However, for Tornado, {{TTornadoStreamTransport.isOpen()}} is not
> implemented, so the call falls back to {{{}TTransportBase{}}}'s empty
> implementation which just returns {{{}None{}}}. As a result, the above
> condition to close the connection never gets satisfied, and my application
> accumulates open connections until eventually it crashes with error message
> {{{}OSError: [Errno 24] Too many open files{}}}.
> To fix this, this patch implements {{{}TTornadoStreamTransport.isOpen(){}}},
> using Tornado's
> [{{BaseIOStream.closed()}}|https://www.tornadoweb.org/en/stable/iostream.html#tornado.iostream.BaseIOStream.closed],
> which returns {{True}} if the stream has been closed. Before checking if the
> stream is open, we need to verify that the stream exists.
> To test the patch, we can add these print statements into
> [thrift/tutorial/py.tornado/PythonClient.py|https://github.com/apache/thrift/blob/2e00c9998f1aa316c8d0168488887fb957845230/tutorial/py.tornado/PythonClient.py#L46]
> in the appropriate places:
> {code:python}
> print(f"transport.isOpen() before transport.open(): {transport.isOpen()}")
> print(f"transport.isOpen() after transport.open(): {transport.isOpen()}")
> print(f"transport.isOpen() after TTransportException: {transport.isOpen()}")
> print(f"transport.isOpen() before transport.close(): {transport.isOpen()}")
> print(f"transport.isOpen() after transport.close(): {transport.isOpen()}")
> {code}
> Without the patch applied, {{isOpen()}} always returns {{{}None{}}},
> regardless of whether the transport is opened or closed:
> {noformat}
> transport.isOpen() before transport.open(): None
> transport.isOpen() after transport.open(): None
> transport.isOpen() before transport.close(): None
> transport.isOpen() after transport.close(): None{noformat}
> {noformat}
> transport.isOpen() before transport.open(): None
> ERROR:root:could not connect to 127.0.0.1:9090 (Stream is closed)
> transport.isOpen() after TTransportException: None{noformat}
> With the patch applied, {{isOpen()}} accurately reflects the transport state:
> {noformat}
> transport.isOpen() before transport.open(): False
> transport.isOpen() after transport.open(): True
> transport.isOpen() before transport.close(): True
> transport.isOpen() after transport.close(): False{noformat}
> {noformat}
> transport.isOpen() before transport.open(): False
> ERROR:root:could not connect to localhost:9090 (Stream is closed)
> transport.isOpen() after TTransportException: False{noformat}
> It also worked neatly with reopening the transport but I didn't include that
> as it didn't fit in neatly with the tutorial's outline.
>
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
