Extend listen-socket to epoll and kqueue transports #755
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The `existing-channel` option added in clj-commons#748 works only with NIO transport (Aleph default). When trying to use it with epoll, we would get a Netty error: `incompatible event loop type: io.netty.channel.epoll.EpollEventLoop`. This is not surprising, since the server-channel class to use depends on the transport, as per [`transport-server-channel-class`](https://github.com/clj-commons/aleph/blob/5a0c6976aa42f30ca3786cef64f9ba70cf037ae1/src/aleph/netty.clj#L1327) and the `existing-channel` handling was always creating a NIO server-channel regardless of transport. It’s not difficult to support most of the other transports. We just have to use the server-channel class as per the aforementioned fn. Their constructors in case of epoll and kqueue take an FD number rather than Java channel. There is no suitable constructor for io-uring, so it is not supported. The validation has been updated accordingly. The full integration test is still limited to NIO, because obtaining an FD of a server-socket opened in Java requires deep reflection (the `--add-opens` flag), which would add some complexity (perhaps a dedicated lein profile) just to test this. The validation test checks that the FD route is attempted for the other transports, but it stops at `Bad file descriptor` error. The option is being renamed from `existing-channel` to `listen-socket`, because this seems more fitting when we are accepting not just channels but also FDs. The `existing-channel` was merged very recently and hasn’t yet been released in a maven version.
PawelStroinski
requested review from
DerGuteMoritz and
arnaudgeiser
as code owners
Collaborator
|
Ah very nice! Glad I didn't yet get around to tagging a new release 😅 Thanks! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The
existing-channeloption added in #748 works only with NIO transport (Aleph default). When trying to use it with epoll, we would get a Netty error:incompatible event loop type: io.netty.channel.epoll.EpollEventLoop.This is not surprising, since the server-channel class to use depends on the transport, as per
transport-server-channel-classand theexisting-channelhandling was always creating a NIO server-channel regardless of transport.It’s not difficult to support most of the other transports. We just have to use the server-channel class as per the aforementioned fn. Their constructors in case of epoll and kqueue take an FD number rather than Java channel. There is no suitable constructor for io-uring, so it is not supported. The validation has been updated accordingly.
The full integration test is still limited to NIO, because obtaining an FD of a server-socket opened in Java requires deep reflection (the
--add-opensflag), which would add some complexity (perhaps a dedicated lein profile) just to test this. The validation test checks that the FD route is attempted for the other transports, but it stops atBad file descriptorerror.The option is being renamed from
existing-channeltolisten-socket, because this seems more fitting when we are accepting not just channels but also FDs. Theexisting-channelwas merged very recently and hasn’t yet been released in a maven version.