Connections fix #27
Connections fix #27
Conversation
- Tracks connections that have been opened so they can be closed. - Tracks background tasks that have been setup so they can be shut down. - Closes connection sockets. - Shuts down background tasks. This was needed to run the test cases as a suite. Previously they only worked individually. Bound client exit to exit function and also improved debug logging. This is a step towards fixing VoltDB#11 but more needs to be done. At least the test suite runs now :)
test/cases/typestest.js
Outdated
| } | ||
| //} else { | ||
| // callback(); | ||
| //} |
There was a problem hiding this comment.
Are these lines commented out by accident or on purpose?
There was a problem hiding this comment.
Hey, previously you couldn't clean up a VoltClient so there was a check to see if the client needed to be created (i.e. create client only if client == null). It's not needed anymore, in fact the test a bit more test-ey (is that a word?) as it creates and tears down the connection a few times. Can prob remove those lines entirely... I guess I left them there as this PR is only one of probably a few more before the client is working nicely. Shall I remove commented lines?
There was a problem hiding this comment.
I see. Thanks for the clarification. I think you can remove them.
lib/client.js
Outdated
| /** | ||
| * TODO: Previously connection only registered once socket established, but it means | ||
| * the connection can't be torn down properly if it doesn't connect. So... it's now registered on | ||
| * init. Need to investigate whether registering the connection earlier will cause any problems. |
There was a problem hiding this comment.
I think the only problem would be when trying to close a connection failed to connect. If any error is handled on close(), it should be fine.
There was a problem hiding this comment.
Change was made specifically for failed connections... i.e. the flush fn would keep firing and keep generating errors and there was no way to stop it. Seems ok now, even with a bad socket.
Can add a bit more defensive code around clean up if you feel would be better?
There was a problem hiding this comment.
Yes, I think that's a good idea. Then you can remove this TODO. Thanks!
… checks in connection close.
|
Hey mate, did the tidy up. Found a typo and fixed that too. |
|
Thanks, @mattyouill ! |
Fix how the client is setup and torn down so that it:
This was needed to run the test cases as a suite. Previously they only worked individually.
Bound client exit to exit function and also improved debug logging.
This is a step towards fixing #11 but more needs to be done. At least the test suite runs now :)