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

Add unix domain socket support #511

Merged
merged 1 commit into from
Oct 19, 2024

Conversation

agorgl
Copy link
Contributor

@agorgl agorgl commented Sep 26, 2024

Similar to PR #478, although this now uses the new Unix Domain Socket Support in Jetty through jetty-unixdomain-server
https://webtide.com/unixdomain-support-in-jetty/

Usage of the new APIs is documented here:
https://jetty.org/docs/jetty/11/programming-guide/server/http.html#connector
https://jetty.org/docs/jetty/11/programming-guide/client/http.html#transport-unix-domain

@agorgl
Copy link
Contributor Author

agorgl commented Sep 26, 2024

Hmm, given that the unix domain sockets require at least java 16 (https://openjdk.org/jeps/380), should I update the pipeline to run the tests using java 17? I think (but i'm not sure) that this won't have any impact in runtime for the users that don't use the :unix-socket option

@weavejester
Copy link
Member

Thanks for the patch. I'd like to be sure that Jetty works over TCP for older versions of the JVM. Can you wrap the failing test in a conditional that checks the JVM version? Then we can potentially check with both.

@agorgl agorgl force-pushed the unix-domain-sockets branch from 3f0357c to 79e26ec Compare September 26, 2024 19:42
@agorgl
Copy link
Contributor Author

agorgl commented Sep 26, 2024

Done! Please run the tests again

@weavejester
Copy link
Member

It looks good. Can you ensure that the lines in this patch are 80 characters or under?

@agorgl agorgl force-pushed the unix-domain-sockets branch from 79e26ec to 3fff1d8 Compare September 27, 2024 06:43
@agorgl
Copy link
Contributor Author

agorgl commented Sep 27, 2024

Fixed a slightly longer def line in the test file, everything else is below 80 chars except from the unix-domain-server-connector function definition that I kept it to 91 to match the format of the server-connector function.
I saw that this slight exception occurs in other places too like the binding of queue-max-capacity in create-threadpool function further down, in favor of consistency.

Copy link
Member

@weavejester weavejester left a comment

Choose a reason for hiding this comment

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

Aside from a small typo in the docstring, this looks ready to merge. Apologies for taking a while to review this.

ring-jetty-adapter/src/ring/adapter/jetty.clj Outdated Show resolved Hide resolved
Signed-off-by: Loukas Agorgianitis <loukas@agorgianitis.com>
@agorgl agorgl force-pushed the unix-domain-sockets branch from 3fff1d8 to df46973 Compare October 18, 2024 15:48
@weavejester weavejester merged commit fef05a2 into ring-clojure:master Oct 19, 2024
1 check passed
@weavejester
Copy link
Member

Thanks for the patch!

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