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

Handle synchronous socket.send error in sendUsingDnsCache #271

Merged
merged 2 commits into from
Sep 13, 2024

Conversation

matteosb
Copy link
Contributor

@matteosb matteosb commented Sep 9, 2024

We are using UDP with DNS caching in production and noticed the occasional unhandled exception in our application when the datadog agent is unavailable. We tracked it down to the lookup branch of sendUsingDnsCache which calls socket.send within a callback. socket.send can throw a sync exception and not invoke callback as described here: https://nodejs.org/api/dgram.html#socketsendmsg-offset-length-port-address-callback.

We are using UDP with DNS caching in production and noticed the occasional unhandled exception in our application when the datadog agent is unavailable. We tracked it down to the lookup branch of `sendUsingDnsCache` which calls `socket.send` within a callback. `socket.send` can throw a sync exception and not invoke `callback` as described here: https://nodejs.org/api/dgram.html#socketsendmsg-offset-length-port-address-callback.
lib/transport.js Outdated Show resolved Hide resolved
@bdeitte
Copy link
Collaborator

bdeitte commented Sep 11, 2024

Nice find! I'd like to see the other two send() calls fixed for this at the same time, unless I'm missing something there. Thanks.

@matteosb
Copy link
Contributor Author

@bdeitte I updated this to just wrap all the socket.send calls for consistency

@bdeitte
Copy link
Collaborator

bdeitte commented Sep 13, 2024

Thanks, merging shortly and making a release

@bdeitte bdeitte merged commit 5b06c56 into brightcove:master Sep 13, 2024
15 checks passed
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