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

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

PawelStroinski
Copy link

@PawelStroinski PawelStroinski commented Apr 5, 2025

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.

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
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).

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.

2 participants