Skip to content

Adopting an existing, pre-bound server-socket channel #748

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Jun 18, 2025
Merged

Adopting an existing, pre-bound server-socket channel #748

merged 1 commit into from
Jun 18, 2025

Conversation

PawelStroinski
Copy link
Contributor

@PawelStroinski PawelStroinski commented Apr 5, 2025 •
edited
Loading

The use-case is to pass a server-socket channel, e.g. obtained by calling JVM's System/inheritedChannel. In this case, Aleph/Netty is not responsible for opening the channel nor closing it. It should simply listen on it. In other words, maintaining the connection sockets is still Aleph/Netty's responsibility, but not maintaining the listening socket if it has been passed to it.

  • In the server startup part, this has been implemented as per Netty's Norman Maurer's advice found here.

  • In the server shutdown part, Aleph is not shutting down nor closing the passed socket, because in practice it's owned by a process that passed it to Aleph process, and it's that parent process' responsibility to reuse or close it after Aleph process exits.

In terms of validation and fallback, for comparison, this used to be Jetty's approach to adopting inheritedChannel at version 8.

  • Jetty automatically falls back to opening its own socket (with a warning) if the inheritedChannel is not supported or not present. In Aleph implementation, it refuses to start the server if the channel type is not supported, or the socket is not bound, but it silently falls back to an own socket if the passed channel is nil. As a result, it's Aleph user's responsibility to trigger an error if it expects an 'inherited channel' which is missing. Still, the user can expect that Aleph will fail early if a channel was actually passed, but it cannot be used.

  • Unlike Jetty, Aleph doesn't itself call configureBlocking false on the channel because this is done by Netty during server startup.

@PawelStroinski
Adopting an existing, pre-bound server-socket channel
82a96c7 
The use-case is to pass a server-socket channel, e.g. obtained by
calling JVM's `System/inheritedChannel`. In this case, Aleph/Netty is
not responsible for opening the channel nor closing it. It should
simply listen on it. In other words, maintaining the connection
sockets is still Aleph/Netty's responsibility, but not maintaining
the listening socket if it has been passed to it.

* In the server startup part, this has been implemented as per Netty's
  Norman Maurer's advice [found here]
  (https://stackoverflow.com/a/50196956).

* In the server shutdown part, Aleph is not shutting down nor closing
  the passed socket, because in practice it's owned by a process that
  passed it to Aleph process, and it's that parent process'
  responsibility to reuse or close it after Aleph process exits.

In terms of validation and fallback, for comparison, [this used to be
Jetty's approach to adopting `inheritedChannel` at version 8]
(https://github.com/jetty/jetty.project/blob/jetty-8.0.0.v20110901/jetty-server/src/main/java/org/eclipse/jetty/server/nio/InheritedChannelConnector.java).

* Jetty automatically falls back to opening its own socket (with a
  warning) if the `inheritedChannel` is not supported or not present.
  In Aleph implementation, it refuses to start the server if the
  channel type is not supported, or the socket is not bound, but it
  silently falls back to an own socket if the passed channel is
  `nil`. As a result, it's Aleph user's responsibility to trigger an
  error if it expects an 'inherited channel' which is missing. Still,
  the user can expect that Aleph will fail early if a channel was
  actually passed, but it cannot be used.

* Unlike Jetty, Aleph doesn't itself call `configureBlocking false` on
  the channel because [this is done by Netty during server startup]
  (https://github.com/netty/netty/blob/46d6e164a2a46e2c6d802f84e0c24abd475bcd89/transport/src/main/java/io/netty/channel/nio/AbstractNioChannel.java#L84).
@KingMob KingMob removed their request for review April 6, 2025 09:16
arnaudgeiser
arnaudgeiser reviewed Apr 10, 2025
View reviewed changes
Copy link
Collaborator

@arnaudgeiser arnaudgeiser left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code-wise it's looking great.
However, I would let @DerGuteMoritz decide whether or not this feature should land on Aleph (to me, it definitely makes sense).

@DerGuteMoritz
Copy link
Collaborator

Nice work! @arnaudgeiser This patch was prompted by a requirement in our project (@PawelStroinski is a co-worker of mine), so I'm naturally in favor of including it 😄 Merging away!

@DerGuteMoritz DerGuteMoritz merged commit 5a0c697 into clj-commons:master Jun 18, 2025
1 check passed
PawelStroinski added a commit to PawelStroinski/aleph that referenced this pull request Jun 21, 2025
@PawelStroinski
Extend listen-socket support to epoll and kqueue transports
aaa00b5 
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 var.
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 added a commit to PawelStroinski/aleph that referenced this pull request Jun 21, 2025
@PawelStroinski
Extend listen-socket to epoll and kqueue transports
2bd3262 
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 added a commit to PawelStroinski/aleph that referenced this pull request Jun 21, 2025
@PawelStroinski
Extend listen-socket to epoll and kqueue transports
064a9cf 
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 PawelStroinski mentioned this pull request Jun 21, 2025
Merged
DerGuteMoritz pushed a commit that referenced this pull request Jun 21, 2025
@PawelStroinski @DerGuteMoritz
Extend listen-socket to epoll and kqueue transports
bb3a1f1 
The `existing-channel` option 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-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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@arnaudgeiser arnaudgeiser arnaudgeiser left review comments

@DerGuteMoritz DerGuteMoritz Awaiting requested review from DerGuteMoritz DerGuteMoritz is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@PawelStroinski @DerGuteMoritz @arnaudgeiser