Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

d-chugunov
Copy link

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
Copy link
Contributor

ipam73 commented Jan 8, 2016

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
Copy link
Author

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

@d-chugunov d-chugunov mentioned this pull request Feb 16, 2016
@azylman
Copy link
Contributor

azylman commented Feb 16, 2016

Closed by #12

@azylman azylman closed this Feb 16, 2016
@d-chugunov
Copy link
Author

@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
Copy link
Contributor

azylman commented Feb 16, 2016

@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

@azylman azylman reopened this Feb 16, 2016
@d-chugunov
Copy link
Author

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
Copy link

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants