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.

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

Fixes #6043 - Reimplement UnixSocket support based on Java 16. #6522

Merged
merged 7 commits into from Aug 5, 2021
Merged

Fixes #6043 - Reimplement UnixSocket support based on Java 16. #6522

merged 7 commits into from Aug 5, 2021

Conversation

sbordet
Copy link
Contributor

@sbordet sbordet commented Jul 16, 2021

  • Introduced new module "jetty-server-unixdomain".
    It uses reflection to access the Java 16 Unix-Domain classes to keep compatibility with the other modules and the build.
  • Added Jetty module with only HTTP/1.1 support for now (requires review of the modules to reuse them with various connectors).
  • Updated documentation to mention UnixDomainServerConnector.
  • Updated client libraries to support Unix-Domain.
  • Updated PROXY protocol implementation to support Unix-Domain.

Signed-off-by: Simone Bordet simone.bordet@gmail.com

@sbordet
Fixes #6043 - Reimplement UnixSocket support based on Java 16.
8806dae 
* Introduced new module "jetty-server-unixdomain".
It uses reflection to access the Java 16 Unix-Domain classes to keep compatibility with the other modules and the build.
* Added Jetty module with only HTTP/1.1 support for now (requires review of the modules to reuse them with various connectors).
* Updated documentation to mention UnixDomainServerConnector.
* Updated client libraries to support Unix-Domain.
* Updated PROXY protocol implementation to support Unix-Domain.

Signed-off-by: Simone Bordet <simone.bordet@gmail.com>
@sbordet sbordet requested review from joakime, gregw and lorban July 16, 2021 15:55
gregw
gregw requested changes Jul 17, 2021
View reviewed changes
Copy link
Contributor

@gregw gregw left a comment

Choose a reason for hiding this comment

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

I'm concerned with hard coding (in config xml) that unix domain is only for raw HTTP. I would expect that other protocols may be carried (eg PROXY from haproxy), so we should set up to be a bit more flexible.

jetty-unixdomain-server/src/main/config/etc/jetty-unixdomain.xml Outdated Show resolved Hide resolved
jetty-unixdomain-server/src/main/config/modules/unixdomain.mod Outdated Show resolved Hide resolved
olamy
olamy requested changes Jul 17, 2021
View reviewed changes
Copy link
Member

@olamy olamy left a comment

Choose a reason for hiding this comment

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

the profile eclipse-release must be updated as well to enforce usage of jdk16 when releasing

pom.xml Show resolved Hide resolved
@sbordet
Removed wrong configuration for Java 16.
431b91f 
Signed-off-by: Simone Bordet <simone.bordet@gmail.com>
@sbordet
Updated tests to be compilable with Java 11 and run on Java 16+.
ff2729b 
Signed-off-by: Simone Bordet <simone.bordet@gmail.com>
@sbordet sbordet requested review from olamy and gregw July 18, 2021 15:47
lorban
lorban requested changes Jul 19, 2021
View reviewed changes
...cumentation/src/main/java/org/eclipse/jetty/docs/programming/server/http/HTTPServerDocs.java Outdated Show resolved Hide resolved
...cumentation/src/main/java/org/eclipse/jetty/docs/programming/server/http/HTTPServerDocs.java Outdated Show resolved Hide resolved
jetty-client/src/main/java/org/eclipse/jetty/client/HttpClient.java Show resolved Hide resolved
jetty-client/src/main/java/org/eclipse/jetty/client/http/HttpClientTransportOverHTTP.java Outdated Show resolved Hide resolved
jetty-io/src/main/java/org/eclipse/jetty/io/ClientConnector.java Show resolved Hide resolved
jetty-io/src/main/java/org/eclipse/jetty/io/ClientConnector.java
channel.socket().setTcpNoDelay(true);
try
{
channel.setOption(StandardSocketOptions.TCP_NODELAY, true);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why forcing this TCP option by default?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are right that this should be configurable, but it's covered by #6372.

jetty-io/src/main/java/org/eclipse/jetty/io/ClientConnector.java Outdated Show resolved Hide resolved
jetty-io/src/main/java/org/eclipse/jetty/io/ClientConnector.java Outdated
/**
* <p>A factory for {@link SocketChannelWithAddress} instances.</p>
*/
public interface Factory
Copy link
Contributor

Choose a reason for hiding this comment

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

This mechanism won't work for QUIC as it works with DatagramChannel instead of SocketChannel.

The SocketChannelWithAddress class could be modified to hold a SelectableChannel instead of SocketChannel but that would not solve the API problem that ClientConnector already exposes SocketChannel via configure(SocketChannel channel) for instance.

This class is sitting between OSI Layer 4 (TCP) and Layer 5 (Sockets, both TCP and Unix Domain), or maybe even overlapping both so I'm not sure I would expose it as an API. Something doesn't look right about it.

Copy link
Contributor

Choose a reason for hiding this comment

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

This should probably be a new issue.

jetty-io/src/main/java/org/eclipse/jetty/io/ClientConnector.java Outdated Show resolved Hide resolved
jetty-server/src/main/java/org/eclipse/jetty/server/ProxyConnectionFactory.java Show resolved Hide resolved
@sbordet
Updates after reviews.
d1797e8 
Signed-off-by: Simone Bordet <simone.bordet@gmail.com>
@sbordet sbordet requested a review from lorban July 19, 2021 18:52
@sbordet
Made the mechanism for creating Unix-Domain sockets and addresses pri…
1b5ce37 
…vate.

This would make easier to fit QUIC in the future.

Signed-off-by: Simone Bordet <simone.bordet@gmail.com>
@sbordet
Copy link
Contributor Author

sbordet commented Jul 27, 2021

@lorban I made the mechanism to create a Unix-Domain ClientConnector more private, as discussed.

I also updated FastCGIProxyServlet to use Unix-Domain when configured so.

lorban
lorban previously approved these changes Jul 29, 2021
View reviewed changes
...cumentation/src/main/java/org/eclipse/jetty/docs/programming/server/http/HTTPServerDocs.java
// The Unix-Domain path to listen to.
connector.setUnixDomainPath(Path.of("/tmp/jetty.sock"));

// The TCP accept queue size.
Copy link
Contributor

Choose a reason for hiding this comment

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

This is unrelated to TCP.

@sbordet
Copy link
Contributor Author

sbordet commented Jul 29, 2021

@gregw @olamy nudge

gregw
gregw previously approved these changes Aug 4, 2021
View reviewed changes
Copy link
Contributor

@gregw gregw left a comment

Choose a reason for hiding this comment

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

better in that out at this stage!

olamy
olamy requested changes Aug 4, 2021
View reviewed changes
Copy link
Member

@olamy olamy left a comment •
edited

Choose a reason for hiding this comment

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

might need to disable some tests for windows OS

jetty-unixdomain-server/src/test/java/org/eclipse/jetty/unixdomain/server/UnixDomainTest.java Show resolved Hide resolved
.../test-distribution/src/test/java/org/eclipse/jetty/tests/distribution/DistributionTests.java Show resolved Hide resolved
@sbordet sbordet linked an issue Aug 4, 2021 that may be closed by this pull request
Reimplement UnixSocket support based on Java 16 #6043
Closed
@sbordet sbordet added this to In progress in Jetty 10.0.7/11.0.7 FROZEN via automation Aug 4, 2021
@sbordet
Issue #6043 - Reimplement UnixSocket support based on Java 16.
82106cf 
Replaced unix.socket.tmp with better named jetty.unixdomain.dir property.
Defaulted jetty.unixdomain.dir property to system property user.home under Windows.
Simplified code that runs Unix-Domain tests.

Signed-off-by: Simone Bordet <simone.bordet@gmail.com>
Jetty 10.0.7/11.0.7 FROZEN automation moved this from In progress to Review in progress Aug 4, 2021
@sbordet
Issue #6043 - Reimplement UnixSocket support based on Java 16.
48ed2df 
Fixed Unix-Domain profile for Windows.
Fixed test to make it work on Windows.

Signed-off-by: Simone Bordet <simone.bordet@gmail.com>
@sbordet sbordet requested a review from olamy August 4, 2021 20:56
joakime
joakime approved these changes Aug 4, 2021
View reviewed changes
Copy link
Contributor

@joakime joakime left a comment

Choose a reason for hiding this comment

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

Tested on Windows 10 laptop, works as expected.

@sbordet
Copy link
Contributor Author

sbordet commented Aug 4, 2021

@olamy this PR now runs fine on Windows too.
There is a new profile to set the Unix-Domain directory for Windows.
Property unix.socket.tmp has been removed in favor of jetty.unixdomain.dir.

Jetty 10.0.7/11.0.7 FROZEN automation moved this from Review in progress to Reviewer approved Aug 5, 2021
@sbordet sbordet merged commit 49a0845 into jetty-10.0.x Aug 5, 2021
Jetty 10.0.7/11.0.7 FROZEN automation moved this from Reviewer approved to Done Aug 5, 2021
@sbordet sbordet deleted the jetty-10.0.x-6043-unixdomain branch August 5, 2021 08:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@olamy olamy olamy approved these changes

@joakime joakime joakime approved these changes

@gregw gregw gregw left review comments

@lorban lorban lorban left review comments

Assignees
No one assigned
Labels
None yet
Projects
No open projects
Jetty 10.0.7/11.0.7 FROZEN
Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Reimplement UnixSocket support based on Java 16
5 participants
@sbordet @olamy @gregw @joakime @lorban