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

Add DNS timeout #1468

Merged
merged 4 commits into from
Sep 28, 2022
Merged

Add DNS timeout #1468

merged 4 commits into from
Sep 28, 2022

Conversation

huksley
Copy link
Contributor

@huksley huksley commented Sep 28, 2022

When DNS server does not respond, nodemailer timeouts only after 150 seconds on Mac OS X

dev Request --> POST /api/preview

Error: queryA ETIMEOUT smtp.sendgrid.net at QueryReqWrap.onresolve [as oncomplete] (node:dns:279:19) {
 (node:dns:279:19) {
  errno: undefined,
  code: 'EDNS',
  syscall: 'queryA',
  hostname: 'smtp.sendgrid.net',
  command: 'CONN'
}

dev Response <-- 500 POST /api/preview Δ 151071ms

This change adds dnsTimeout which is passed to dns [1] NodeJS module.

@andris9
Copy link
Member

andris9 commented Sep 28, 2022

In general, I would accept this change. The issue is that Nodemailer has a long-term commitment to backwards compatibility, and your changes do not work with older versions of Node.js. Could you make this conditional so that if there is no Resolver class available, then Nodemailer would not try to use it?

CI does not work with older versions of Node, so you can check if the code works in general like this:

$ nvm use 6
$ export NODE_TLS_REJECT_UNAUTHORIZED="0"
$ node examples/full.js

Currently it ends with

TypeError: dns.Resolver is not a constructor
    at resolver (/root/nodemailer/lib/shared/index.js:51:22)
    at Object.module.exports.resolveHostname (/root/nodemailer/lib/shared/index.js:126:5)

@huksley
Copy link
Contributor Author

huksley commented Sep 28, 2022

@andris9 Done. Thanks for a test code which is easy to run and test with. Tested on nvm use 6and nvm use 16

@andris9 andris9 merged commit e091992 into nodemailer:master Sep 28, 2022
@huksley
Copy link
Contributor Author

huksley commented Sep 28, 2022

🎉 @andris9 Please advise, how to improve types for nodemailer? it is 6.4.6 vs nodemailer@6.7.x, should I find all changes between those two version or I can only bump types to 6.7.x adding dnsTimeout?

@andris9
Copy link
Member

andris9 commented Sep 28, 2022

Well, I know nothing about typings, these are not part of the Nodemailer project but are fully community-managed. Most of the code in Nodemailer is written years before Typescript was invented 🤷‍♂️

@andris9
Copy link
Member

andris9 commented Sep 28, 2022

Published changes to npm as nodemailer@6.8.0

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.

None yet

2 participants