Skip to content

Comments

Enable TCP keepalives for early detection of severed tcp connections (issue #1)#40

Merged
ExploreMqt merged 5 commits intologicalparadox:masterfrom
nrcmedia:keepalives
Jan 25, 2017
Merged

Enable TCP keepalives for early detection of severed tcp connections (issue #1)#40
ExploreMqt merged 5 commits intologicalparadox:masterfrom
nrcmedia:keepalives

Conversation

@mvdklip
Copy link
Contributor

@mvdklip mvdklip commented Dec 21, 2016

Enabling TCP keepalives fixed issue #1 for us. In our case a firewall managed by our provider was silently dropping packets for TCP connections which had been idle for more than an hour. Packets could be seen piling up in the 'Send-Q' column of netstat in this case and notifications would be lost at some point when the TCP stack finally gave up and emitted an ETIMEDOUT error. TCP keepalives fool the firewall and prevent the connection from becoming idle.

@ExploreMqt
Copy link
Collaborator

A question and a request.
Why did you want to revert setting the meta data?
Please make the delay configurable from the outside and don't call setKeepAlive if it is zero. There should be a test for this too.

@mvdklip
Copy link
Contributor Author

mvdklip commented Dec 21, 2016

The revert was an error on my side. Thought I reverted a change done by a previous developer working for our company which was no longer needed. I did not notice it had been pulled upstream. Sorry for that. I reverted the revert. :)

Added a commit which makes the delay configurable as requested. Not sure how to add a test for this.

@ExploreMqt
Copy link
Collaborator

Just checking in. Were you planning on writing the unit tests?

@mvdklip
Copy link
Contributor Author

mvdklip commented Dec 28, 2016

Added test for the sake of it. I am unable to actually reproduce/test scenarios for which this was designed, but the minimum I could do was to test that the 'keepalive delay' config var is actually being set and it accepts both strings as well as numbers (0 to disable).

@mvdklip
Copy link
Contributor Author

mvdklip commented Jan 18, 2017

Have added tests as requested. Can you accept this PR in its current state or do you need something else?

@ExploreMqt
Copy link
Collaborator

Sorry, I got distracted with the holidays and then forgot.
Actually I was hoping for some testing around the logic of setting/not setting of setKeepAlive and the computations of values passed to it. Unit tests are for validating logic and you added behavior.

@mvdklip
Copy link
Contributor Author

mvdklip commented Jan 20, 2017

I have looked at this again and again but I'm afraid I don't see what else could be tested here. Currently it tests the case of not setting the value and checking the default of '5m' in this case and the case of disabling the behaviour by setting a value of 0. Implementing the latter actually lead to discovering that the previous implementation was only accepting string values, so that test actually already proved itself somewhat. I could add another test setting yet another arbitrary value, but the thing is that I can only access set() and get() from the test so it seems kind of pointless.

Computations of values passed to setKeepAlive are done by the ms module so I'd say that should be covered by the unit tests for that module, right?

That said I'm more familiar with functional (black box) testing than I am with unit testing of software so I might just not get it. :-P

@ExploreMqt ExploreMqt merged commit 3a68f53 into logicalparadox:master Jan 25, 2017
@mvdklip mvdklip deleted the keepalives branch January 25, 2017 09:00
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.

2 participants