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

Fix uds reconnection logic #188

Closed

Conversation

prognant
Copy link

Statsd UDS reconnection logic is not working because error code are checked against positive value : current codebase use the following positive errno : 111 ECONNREFUSED & 107 ENOTCONN.

However node.js relies on libuv that uses negated errno :

This PR uses util.getSystemErrorName that was made available in node.js v9.7.0 and subsequently backported to v8.12.0

Either it's acceptable to bump the node.js version requirements from "Node 8.x and higher" to "Node 8.12 and higher" or we can go back to the hardcoded number and just negate those (side notes: those codes does not exit on mac and are different on windows, -4053 and -4078 thus resolving to the text representation will be more platform agnostic) just let me know !

@bdeitte
Copy link
Collaborator

bdeitte commented Sep 26, 2020

@prognant Thank you, well that explains why there were issues here! Two things to note on this:

  1. I would really like to still have Node 8 and above, just to not surprise anybody with a lot of usage of this library. And can note in the code to update later
  2. There's another similar PR, funny timing on having these both: https://github.com/brightcove/hot-shots/pull/189/files. I haven't had a chance yet to figure out yet what makes sense to merge, if they both have different functionality, etc. So interesting in both your and @dhermes thoughts on this

@dhermes
Copy link
Contributor

dhermes commented Sep 26, 2020

@bdeitte This PR fixes 1 of the 4 issues I mentioned in #128 (comment) and is a good start. It could also be done (as a quick hack) by just swapping 107 for -107 and 111 for -111.

Unfortunately a UDS failure with ENOTCONN / ECONNREFUSED is Linux specific and on macOS those errors are EDESTADDRREQ / ECONNRESET.

I actually initially had used util.getSystemErrorName() but later decided against it because it can throw an error on bad inputs and the error codes are nicely available any how via os.constants.errno (I haven't checked how far back they are available, I only checked Node 12 on macOS, Debian and Alpine.)

@prognant
Copy link
Author

Hi @dhermes & @bdeitte, thanks for your prompt replies.

The PR #189 seems to be a better fit overall. BTW the os.constants thing is available at least since node.js 6 as far as I could test, so that's probably the best way not to hardcode errno value.

So I'll close this one and add my thoughts, if any, on the other PR !

Thanks a lot !

@prognant prognant closed this Sep 28, 2020
@dhermes
Copy link
Contributor

dhermes commented Sep 28, 2020

@prognant You made some remarks about Windows. Have you done any testing / confirmed how UDS failures look on Windows?

@bdeitte
Copy link
Collaborator

bdeitte commented Sep 30, 2020

Thanks @prognant for taking a look here and your thoughts!

@prognant
Copy link
Author

prognant commented Sep 30, 2020

@prognant You made some remarks about Windows. Have you done any testing / confirmed how UDS failures look on Windows?

Hi @dhermes, UDS support is fairly recent on Windows (https://devblogs.microsoft.com/commandline/af_unix-comes-to-windows/) and AFAIK datagram are not yet supported. The one thing that comes to my mind regarding Windows is DataDog/datadog-agent#5809 that leverages a Windows specific thing (windows named pipe) that behaves similarly to UDS. Node.js seems to natively support windows named pipe, but that would probably be a new Windows-only feature and the whole UDS thing is likely to remain a linux/bsd/mac thing.

@dhermes
Copy link
Contributor

dhermes commented Sep 30, 2020

@prognant Awesome info, thanks for the explanation!

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.

3 participants