search

Clever / thrift-pool

A module that wraps thrift interfaces in connection pooling logic to make them more resilient.
Apache License 2.0
22 stars 15 forks source link

Fixed issue when connection object is not released after error or tim… #9

Open d-chugunov opened 8 years ago

d-chugunov commented 8 years ago

You can reproduce the issue following next steps. My thrift server works with TBufferedTransportFactory and TBinaryProtocolFactory, but I (accidentally) created node.js thrift client connection with protocol == null and transport == TFramedTransport. As a result the server closes the connection, but node.js library can't detect it. So I had added one second timeout and got timeout error after the first query to thrift server. But the second query hung and returned neither error nor result because the previous connection was dead but it hadn't been recreated by pool because it had been considered as used.

ipam73 commented 8 years ago

Thanks for this change @d-chugunov. (We left some comments on the SSL change) In regards to this change, what are your thoughts on mocking some of this behavior and adding some tests?

d-chugunov commented 8 years ago

@ipam73 Actually I am not acquainted with coffeescript, therefore I would have some troubles with making tests.

azylman commented 8 years ago

Closed by #12

d-chugunov commented 8 years ago

@azylman Why don't you consider adding remove_listeners connection, cb_error, cb_timeout, cb_close pool.release connection in cb_error, cb_timeout and cb_close callbacks? They must be there for proper working.

azylman commented 8 years ago

@d-chugunov Ah, sorry, didn't notice that there was other stuff in this PR. I tried to reproduce the problem you were seeing and wasn't able to, so I wasn't ready to merge this in yet, but don't want to close it

d-chugunov commented 8 years ago

Hi. Sorry, I had no time to continue discussing the issue. So here is my example https://github.com/d-chugunov/TestThriftPool The folder nodejs contains Node.js client code (look in nodejs/bin/www) and the python - python 2.7 server code (look in python/server.py). As you can see my server works using TBufferedTransport, but in client code I've intentionally made a mistake: I use TFramedTransport. As a result I get the following output: Mon Mar 21 2016 11:09:01 GMT+0300 (MSK): making ping... err = Error: Thrift-pool: Connection timeout response = undefined Mon Mar 21 2016 11:09:02 GMT+0300 (MSK): making ping... Mon Mar 21 2016 11:09:03 GMT+0300 (MSK): making ping... Mon Mar 21 2016 11:09:04 GMT+0300 (MSK): making ping... Mon Mar 21 2016 11:09:05 GMT+0300 (MSK): making ping... As you can see I get neither error nor response on second and further queries. But after applying my fix to your library everything works as it should do.

DanielYWoo commented 8 years ago

I have similar problem with thrift-pool, when the server close connection due to an exception the node client cannot destroy the connection or return the connection back to the pool properly. My fix is very similar to the solution suggested by @d-chugunov