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 ESERVFAIL crash on timeout NS #90

Closed
wants to merge 2 commits into from
Closed

Conversation

analogic
Copy link

@analogic analogic commented Jun 4, 2024

No description provided.

@coveralls
Copy link

Pull Request Test Coverage Report for Build 9362569134

Details

  • 1 of 1 (100.0%) changed or added relevant line in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.01%) to 91.083%

Totals Coverage Status
Change from base Build 8931345490: 0.01%
Covered Lines: 650
Relevant Lines: 708

💛 - Coveralls

@msimerson
Copy link
Member

This doesn't fix the issue. The two DNS errors that are "non-fatal" essentially mean, "try again but do an A query instead of MX", which will may resolve to an implicit MX. In the case of a SERVFAIL, that's almost certainly going to fail as well (which it does):

❯ drill kamenoprumysl.cz MX
;; ->>HEADER<<- opcode: QUERY, rcode: SERVFAIL, id: 60612
;; flags: qr rd ra ; QUERY: 1, ANSWER: 0, AUTHORITY: 0, ADDITIONAL: 0 
;; QUESTION SECTION:
;; kamenoprumysl.cz.	IN	MX

;; ANSWER SECTION:

;; AUTHORITY SECTION:

;; ADDITIONAL SECTION:

;; Query time: 1858 msec
;; WHEN: Tue Jun  4 09:14:51 2024
;; MSG SIZE  rcvd: 34

and then...

❯ drill kamenoprumysl.cz A
;; ->>HEADER<<- opcode: QUERY, rcode: SERVFAIL, id: 32247
;; flags: qr rd ra ; QUERY: 1, ANSWER: 0, AUTHORITY: 0, ADDITIONAL: 0 
;; QUESTION SECTION:
;; kamenoprumysl.cz.	IN	A

;; ANSWER SECTION:

;; AUTHORITY SECTION:

;; ADDITIONAL SECTION:

;; Query time: 4315 msec
;; WHEN: Tue Jun  4 09:14:58 2024
;; MSG SIZE  rcvd: 34

@analogic
Copy link
Author

analogic commented Jun 4, 2024

I don't really understand why, but it doesn't crash anymore, even a bounce with the message "Error: Nowhere to deliver mail to for domain: kamenoprumysl.cz" is generated :)

So there must be a place where err leaks and backtrace does not help. Hmm

@msimerson
Copy link
Member

I don't really understand why

I expect it's because your DNS server has cached the MX lookup failure, so the subsequent A query fails faster.

So there must be a place where err leaks and backtrace does not help.

I'm assuming that you don't have any get_mx hook returning for kamenoprumysl.cz and so the entry point to get_mx is here. If the promise times out, .then won't get called but the .catch will and get_mx_error will run. Or it should.

@analogic
Copy link
Author

analogic commented Jun 4, 2024

Could be the reason that error handling should not use the class's functions, e.g. logging?

2024-06-04 20:06:43.655883500  [CRIT] [-] [core] TypeError: Cannot read properties of undefined (reading 'lognotice')
2024-06-04 20:06:43.655923500  [CRIT] [-] [core]     at get_mx_error (/usr/lib/node_modules/Haraka/outbound/hmail.js:246:14)
2024-06-04 20:06:43.656750500  [NOTICE] [-] [core] Shutting down

Sometimes I really don't like javascript, it makes me feel like I'm not intelligent enough.

@analogic
Copy link
Author

analogic commented Jun 4, 2024

closing in favour of haraka/Haraka#3376

@analogic analogic closed this Jun 4, 2024
@msimerson
Copy link
Member

Sometimes I really don't like javascript, it makes me feel like I'm not intelligent enough.

I know that feeling. But it's mostly that older/legacy JS has an abundance of rough edges. I find writing newer JS to be less onerous.

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