Add tests for dns unreachable and fix hang by epinter · Pull Request #28 · apache/james-jspf

epinter · 2025-01-22T16:30:10Z

This PR adds tests to reproduce DNS unreachable condition which causes a hang when async executor is in use, works flawlessly with sync.

epinter · 2025-01-22T17:09:52Z

Test running in Jenkins is really returning "none" ? Anybody knows why ? Mine returns "temperror", running on linux.

14:05:46.066 [ERROR] o.a.j.j.i.SPF - null
java.net.PortUnreachableException: null
	at java.base/sun.nio.ch.DatagramDispatcher.read0(Native Method)
	at java.base/sun.nio.ch.DatagramDispatcher.read(DatagramDispatcher.java:43)
	at java.base/sun.nio.ch.IOUtil.readIntoNativeBuffer(IOUtil.java:276)
	at java.base/sun.nio.ch.IOUtil.read(IOUtil.java:245)
	at java.base/sun.nio.ch.IOUtil.read(IOUtil.java:223)
	at java.base/sun.nio.ch.DatagramChannelImpl.read(DatagramChannelImpl.java:610)
	at org.xbill.DNS.NioUdpClient$Transaction.processReadyKey(NioUdpClient.java:122)
	at org.xbill.DNS.NioClient.processReadyKeys(NioClient.java:182)
	at org.xbill.DNS.NioClient.runSelector(NioClient.java:134)
	at java.base/java.lang.Thread.run(Thread.java:829)
temperror

Received-SPF: temperror (spfCheck: Error in retrieving data from DNS) client-ip=207.54.72.202; envelope-from=do_not_reply@reyifglerifwukfvbdjhrkbvebvekvfulervkerkeruerbeb.de; helo=reyifglerifwukfvbdjhrkbvebvekvfulervkerkeruerbeb.de;

Jenkins output:

org.junit.ComparisonFailure: expected:<[temperror]> but was:<[none]>
	at org.apache.james.jspf.SynchronousSPFExecutorIntegrationTest.shouldReturnTempErrorOnPortUnreachable(SynchronousSPFExecutorIntegrationTest.java:96)

Setting default resolver
none

Received-SPF: none (spfCheck: 207.54.72.202 is neither permitted nor denied by domain of reyifglerifwukfvbdjhrkbvebvekvfulervkerkeruerbeb.de) client-ip=207.54.72.202; envelope-from=do_not_reply@reyifglerifwukfvbdjhrkbvebvekvfulervkerkeruerbeb.de; helo=reyifglerifwukfvbdjhrkbvebvekvfulervkerkeruerbeb.de;

NVM: It was the cache

epinter · 2025-01-22T22:28:22Z

I've found another hang, but It's hard to create a test and depend on real DNS. I will open another PR for you to review the solution, because I will add a class to simulate a real DNS.

chibenwa

Thanks for the GREAT contribution, it looks awesome!

chibenwa · 2025-01-23T07:48:59Z

+        System.out.println(result.getResult());
+        System.out.println(result.getExplanation());
+        System.out.println(result.getHeader());


Maybe we can get rid of those System.out.println ?

chibenwa · 2025-01-23T07:49:19Z

+        System.out.println(result.getResult());
+        System.out.println(result.getExplanation());
+        System.out.println(result.getHeader());


In some tests like these, the details helps sometimes. Do you prefer I replace all these println with logger instead of remove them ?

We could replace those with assertions so that the test reader get it in the source code of the test?

Right. I will replace with assert the most relevant, and remove others.

chibenwa · 2025-01-23T08:03:58Z

(on a side note I would like to really thank you for your contributions to JSPF. Your help is much appreciated.
Feel free to contact me on btellier@apache.org, I'd be happy to discuss your use cases, and discuss your contributions to JSPF).

epinter · 2025-01-23T16:08:10Z

(on a side note I would like to really thank you for your contributions to JSPF. Your help is much appreciated. Feel free to contact me on btellier@apache.org, I'd be happy to discuss your use cases, and discuss your contributions to JSPF).

Thank you :)
I'm a user of the library, and while I see I can contribute with something I will. It's a way to recognize all the work you and your team put in software like this.

The tests with hostname without dot are beign ignored.

When a PortUnreachableException or SocketException happens, a temperror should be returned.

This NPE happens when a PortUnreachableException (that doesn't have a message) is received in the exceptionally block, but other exceptions without a message also can trigger this bug. Also added the null check for e.getCause(), just in case.

Situations like a port unreachable should be considered a temperror by async executor, like the sync does.

epinter · 2025-01-24T03:15:14Z

Sorry for the force-push after the approval, but the commit "Fix tests hostname so they are not skipped" had an error, I didn't want it to be merged and have to create another fix after...

chibenwa

No worry, the change set is rather small and still easy to understand (good commit history).

epinter · 2025-02-06T17:28:10Z

Hi !
Do you know when the PR will be merged ? I have 2 more bugs I found on Async Executor, but I want to send another PR because I don't know if you will approve the method I used to test.

chibenwa · 2025-02-06T20:36:33Z

Hello,
Sorry it pretty much went out of the radar...
I guess I did not merged it straight away to let others a chance to review.

I have 2 more bugs I found on Async Executor, but I want to send another PR because I don't know if you will approve the method I used to test.

Please go ahead!

epinter · 2025-02-06T21:24:55Z

Hello, Sorry it pretty much went out of the radar... I guess I did not merged it straight away to let others a chance to review.

I have 2 more bugs I found on Async Executor, but I want to send another PR because I don't know if you will approve the method I used to test.

Please go ahead!

No problem. Thanks !

epinter marked this pull request as draft January 22, 2025 16:30

epinter marked this pull request as ready for review January 22, 2025 16:53

epinter force-pushed the fix-loop branch from 5dc6fc9 to 94a9b0c Compare January 22, 2025 16:58

epinter mentioned this pull request Jan 22, 2025

Fixed asynchronous hanging w/truncated SPF record #25

Open

epinter force-pushed the fix-loop branch from d962931 to 3bbc2d8 Compare January 23, 2025 07:17

epinter changed the title ~~Add tests for dns unreachable and fix infinite loop~~ Add tests for dns unreachable and fix hang Jan 23, 2025

chibenwa reviewed Jan 23, 2025

View reviewed changes

Arsnael approved these changes Jan 24, 2025

View reviewed changes

epinter added 6 commits January 23, 2025 23:59

Fix tests hostname so they are not skipped

b4159b8

The tests with hostname without dot are beign ignored.

Add tests for cases when the dns is unreachable

5038fd6

When a PortUnreachableException or SocketException happens, a temperror should be returned.

[JSPF-115] Fix NPE that causes hang

7a231a2

This NPE happens when a PortUnreachableException (that doesn't have a message) is received in the exceptionally block, but other exceptions without a message also can trigger this bug. Also added the null check for e.getCause(), just in case.

The result from async should be temperror when a SocketException happens

96dae3d

Situations like a port unreachable should be considered a temperror by async executor, like the sync does.

Clear dnsjava cache before each test

0cd57fc

Cleanup tests

ec1a5e0

epinter force-pushed the fix-loop branch from 4d7ef46 to ec1a5e0 Compare January 24, 2025 03:13

chibenwa approved these changes Jan 24, 2025

View reviewed changes

chibenwa merged commit 862496e into apache:master Feb 6, 2025

Conversation

epinter commented Jan 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

epinter commented Jan 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

epinter commented Jan 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chibenwa left a comment

Choose a reason for hiding this comment

Uh oh!

chibenwa Jan 23, 2025

Choose a reason for hiding this comment

Uh oh!

chibenwa Jan 23, 2025

Choose a reason for hiding this comment

Uh oh!

epinter Jan 23, 2025

Choose a reason for hiding this comment

Uh oh!

chibenwa Jan 23, 2025

Choose a reason for hiding this comment

Uh oh!

epinter Jan 23, 2025

Choose a reason for hiding this comment

Uh oh!

chibenwa commented Jan 23, 2025

Uh oh!

epinter commented Jan 23, 2025

Uh oh!

epinter commented Jan 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chibenwa left a comment

Choose a reason for hiding this comment

Uh oh!

epinter commented Feb 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chibenwa commented Feb 6, 2025

Uh oh!

epinter commented Feb 6, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

epinter commented Jan 22, 2025 •

edited

Loading

epinter commented Jan 22, 2025 •

edited

Loading

epinter commented Jan 22, 2025 •

edited

Loading

epinter commented Jan 24, 2025 •

edited

Loading

epinter commented Feb 6, 2025 •

edited

Loading