Add support for Unix domain datagram sockets when using native epoll/kqueue transport by violetagg · Pull Request #11423 · netty/netty

violetagg · 2021-06-29T15:26:08Z

Motivation:

There are use cases when Unix domain datagram sockets are needed for communication. This PR adds such support for Epoll/KQueue.

Modification:

Expose Channel, Config and Packet interfaces/classes for Unix domain datagram sockets. All interfaces/classes are in transport-native-unix-common module in order to be available for KQueue and Epoll implementations
Add JNI code for Unix domain datagram sockets
Refactor DatagramUnicastTest so that it can be used for testing also Unix domain datagram sockets
Add Unix domain datagram sockets implementation for KQueue transport
Add Unix domain datagram sockets implementation for Epoll transport

Result:

Fixes #6737

violetagg · 2021-06-29T15:33:15Z

Here are some notes for the current implementation:

I tried to separate the changes on logical commit so that it is easier for a review.
There isn't File descriptor support:

what might be the use case?
there is a possibility the packet to be discarded.

There is no disconnect support as I wasn't able to have it working with AF_UNSPEC for both KQueue and Epoll

chrisvest

Had some random comments from a quick glance. Haven't done a full review.

normanmaurer · 2021-07-05T07:15:30Z

@violetagg can you please rebase ?

violetagg · 2021-07-06T07:39:19Z

The build on Windows failed with the one below, I'm not sure how this is related to this change

2021-07-05T09:07:34.2974217Z [ERROR] Failures: 
2021-07-05T09:07:34.2976021Z [ERROR]   JdkOpenSslEngineInteroptTest>SSLEngineTest.testMutualAuthDiffCerts:582->SSLEngineTest.runTest:1191->SSLEngineTest.writeAndVerifyReceived:1220
2021-07-05T09:07:34.2979428Z [INFO] 
2021-07-05T09:07:34.2980299Z [ERROR] Tests run: 12074, Failures: 1, Errors: 0, Skipped: 344

normanmaurer · 2021-07-06T08:21:44Z

@violetagg yeah the test-failure was observed before... trying to reproduce it as well

normanmaurer

I think this is mostly fine... Just a few comments

normanmaurer · 2021-07-08T12:47:42Z

Yes

…

Am 08.07.2021 um 14:43 schrieb Violeta Georgieva ***@***.***>: @violetagg commented on this pull request. In transport-native-kqueue/src/main/java/io/netty/channel/kqueue/KQueueDomainDatagramChannel.java: > +import io.netty.channel.unix.DomainDatagramSocketAddress; +import io.netty.channel.unix.DomainSocketAddress; +import io.netty.channel.unix.IovArray; +import io.netty.channel.unix.PeerCredentials; +import io.netty.channel.unix.UnixChannelUtil; +import io.netty.util.CharsetUtil; +import io.netty.util.UncheckedBooleanSupplier; +import io.netty.util.internal.StringUtil; + +import java.io.IOException; +import java.net.SocketAddress; +import java.nio.ByteBuffer; + +import static io.netty.channel.kqueue.BsdSocket.newSocketDomainDgram; + +public final class KQueueDomainDatagramChannel extends AbstractKQueueDatagramChannel implements DomainDatagramChannel { @normanmaurer This @UnstableApi should apply also to the corresponding EpollDomainDatagramChannel and EpollDomainDatagramChannelConfig ? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe.

normanmaurer · 2021-07-09T04:55:49Z

@violetagg can you rebase this one ?

…datagram sockets All interfaces/classes are in transport-native-unix-common module in order to be available for KQueue and Epoll impl

Motivation: Move INET related test in a separate test class, with that change DatagramUnicastTest can be used for testing also Unix domain datagram sockets. Modifications: - Move DatagramUnicastTest#testBindWithPortOnly to DatagramUnicastInetTest. - Adapt DatagramUnicastIPv6Test, EpollDatagramUnicastTest and KQueueDatagramUnicastTest to inherit the new test class. - testBindWithPortOnly(Bootstrap, Bootstrap) method tests only with the second parameter - remove from the signature the first parameter. Make the method private and static. Result: DatagramUnicastTest can be used for testing also Unix domain datagram sockets.

…Unix domain datagram sockets.

Make DatagramUnicastTest an abstract class

According to https://man7.org/linux/man-pages/man7/unix.7.html SO_RCVBUF option does not effect for UNIX domain sockets "The SO_SNDBUF socket option does have an effect for UNIX domain sockets, but the SO_RCVBUF option does not."

Make the helper test methods private

violetagg · 2021-07-09T05:23:58Z

@violetagg can you rebase this one ?

ok, rebased

trustin · 2021-07-09T07:32:20Z

I went over the PR and it seems good to me. Didn't have enough time to review every single line, though. One concern is we have quite a bit of duplication across the channel implementation such as spinning I/O, but that's probably something that has to be handled in other place.

normanmaurer · 2021-07-09T07:35:36Z

@trustin yeah I think this is something to fix later on.

violetagg · 2021-07-09T07:36:35Z

I went over the PR and it seems good to me. Didn't have enough time to review every single line, though. One concern is we have quite a bit of duplication across the channel implementation such as spinning I/O, but that's probably something that has to be handled in other place.

I refrained from doing a big refactoring because it would complicate the review process

normanmaurer · 2021-07-09T07:45:56Z

@violetagg thanks a lot!

violetagg · 2021-07-09T07:55:38Z

@chrisvest @trustin @normanmaurer Thank you very much! 🎉 🚀

normanmaurer · 2021-07-09T14:13:53Z

@violetagg I tried to port this over to master but I have some failing tests... Not sure yet why :/

violetagg · 2021-07-09T14:43:42Z

@violetagg I tried to port this over to master but I have some failing tests... Not sure yet why :/

Should I try?

normanmaurer · 2021-07-09T15:17:27Z

Yes please

…

Am 09.07.2021 um 16:43 schrieb Violeta Georgieva ***@***.***>: @violetagg I tried to port this over to master but I have some failing tests... Not sure yet why :/ Should I try? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe.

violetagg · 2021-07-09T15:19:35Z

Yes please
…

ok let me give it a try

violetagg · 2021-07-10T09:06:10Z

@normanmaurer I opened this PR #11476
Let's see whether the tests will be OK (on my side they are passing)

…kqueue transport (netty#11423) Motivation: There are use cases when Unix domain datagram sockets are needed for communication. This PR adds such support for Epoll/KQueue. Modification: - Expose Channel, Config and Packet interfaces/classes for Unix domain datagram sockets. All interfaces/classes are in `transport-native-unix-common` module in order to be available for KQueue and Epoll implementations - Add JNI code for Unix domain datagram sockets - Refactor `DatagramUnicastTest` so that it can be used for testing also Unix domain datagram sockets - Add Unix domain datagram sockets implementation for KQueue transport - Add Unix domain datagram sockets implementation for Epoll transport Result: Fixes netty#6737

violetagg commented Jun 29, 2021

View reviewed changes

Comment thread ...port-native-epoll/src/main/java/io/netty/channel/epoll/EpollDomainDatagramChannelConfig.java Outdated

violetagg mentioned this pull request Jun 29, 2021

Support DGRAM unix domain socket #6737

Closed

chrisvest reviewed Jul 2, 2021

View reviewed changes

normanmaurer requested a review from chrisvest July 8, 2021 12:03

normanmaurer requested changes Jul 8, 2021

View reviewed changes

violetagg added 9 commits July 9, 2021 08:22

Expose Channel, Config and Packet interfaces/classes for Unix domain …

da3a270

…datagram sockets All interfaces/classes are in transport-native-unix-common module in order to be available for KQueue and Epoll impl

Add JNI code for Unix domain datagram sockets

e3cd647

Refactor DatagramUnicastTest so that it can be used for testing also …

78b1819

…Unix domain datagram sockets.

Add Unix domain datagram sockets support for KQueue transport

8964996

Add Unix domain datagram sockets support for Epoll transport

17b2e4d

Move INET related functionality to DatagramUnicastInetTest

6814e4d

Make DatagramUnicastTest an abstract class

Remove SO_RCVBUF from the configuration options

1033ce8

According to https://man7.org/linux/man-pages/man7/unix.7.html SO_RCVBUF option does not effect for UNIX domain sockets "The SO_SNDBUF socket option does have an effect for UNIX domain sockets, but the SO_RCVBUF option does not."

Mark the API as unstable

b2fb74d

Make the helper test methods private

normanmaurer added this to the 4.1.66.Final milestone Jul 9, 2021

normanmaurer approved these changes Jul 9, 2021

View reviewed changes

normanmaurer merged commit 3f9a5f5 into netty:4.1 Jul 9, 2021

violetagg deleted the uds-dgram branch July 9, 2021 15:20

vietj mentioned this pull request Jul 27, 2021

Add support for Unix domain datagram sockets when using native epoll/kqueue transport eclipse-vertx/vert.x#4038

Open

Eng-Fouad mentioned this pull request Jun 1, 2024

Support for Unix Domain DatagramSocket eclipse-vertx/vert.x#5218

Open

Uh oh!

Conversation

violetagg commented Jun 29, 2021

Uh oh!

violetagg commented Jun 29, 2021

Uh oh!

Uh oh!

chrisvest left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

normanmaurer commented Jul 5, 2021

Uh oh!

violetagg commented Jul 6, 2021

Uh oh!

normanmaurer commented Jul 6, 2021

Uh oh!

normanmaurer left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

normanmaurer commented Jul 8, 2021 via email

Uh oh!

normanmaurer commented Jul 9, 2021

Uh oh!

violetagg commented Jul 9, 2021

Uh oh!

trustin commented Jul 9, 2021

Uh oh!

normanmaurer commented Jul 9, 2021

Uh oh!

violetagg commented Jul 9, 2021

Uh oh!

normanmaurer commented Jul 9, 2021

Uh oh!

violetagg commented Jul 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

normanmaurer commented Jul 9, 2021

Uh oh!

violetagg commented Jul 9, 2021

Uh oh!

normanmaurer commented Jul 9, 2021 via email

Uh oh!

violetagg commented Jul 9, 2021

Uh oh!

violetagg commented Jul 10, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

violetagg commented Jul 9, 2021 •

edited

Loading