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 support for unix domain sockets (APR Protocol) #401

Closed
wants to merge 1 commit into from

Conversation

minfrin
Copy link
Contributor

@minfrin minfrin commented Jan 15, 2021

Add support and test case for Unix Domain Sockets in
org.apache.coyote.http11.Http11AprProtocol, to support
users using Java versions older than 16.

Requires tomcat-native 1.2.26 and above.

https://bz.apache.org/bugzilla/show_bug.cgi?id=64943

@michael-o
Copy link
Member

Give me a couple of days and I will run this on FreeBSD and HP-UX.

@michael-o michael-o self-requested a review January 15, 2021 17:24
@rmaucher
Copy link
Contributor

I still think it is a bad idea to continue to promote the APR connector. At least domain socket support won't be an excuse to keep it around longer than absolutely necessary.

Anyway, if this ends up being included, I will then refactor the feature a bit more.

Copy link
Member

@michael-o michael-o left a comment

Choose a reason for hiding this comment

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

A few minor issues, but on both FreeBSD and HP-UX the tests with APR 1.6.6 and 1.7.0 passed for me.

Copy link
Member

@michael-o michael-o left a comment

Choose a reason for hiding this comment

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

LGTM. Square, rebase and it is fine for merging.
@rmaucher Can you add your changes in top of these for review after @minfrin has completed his work. We could review and merge them back.

Add support and test case for Unix Domain Sockets in
org.apache.coyote.http11.Http11AprProtocol, to support
users using Java versions older than 16.

Requires tomcat-native 1.2.26 and above.
@minfrin
Copy link
Contributor Author

minfrin commented Jan 26, 2021

In theory 96e241c should do the trick, can you confirm this is ready to merge?

@michael-o
Copy link
Member

For me yes, but I want Rémy give a chance to do his improvements on top.

@rmaucher
Copy link
Contributor

I don't plan any improvements or changes at this time.

@michael-o
Copy link
Member

Alright, I will merge this this week.

@minfrin
Copy link
Contributor Author

minfrin commented Jan 29, 2021

Thanks for this.

@michael-o
Copy link
Member

@minfrin I have now pushed this to master. Can you please backport to 9.0.x and 8.5.x and then I will merge them too?

@martin-g
Copy link
Member

martin-g commented Feb 1, 2021

@michael-o Are new PRs needed ? We can cherry-pick the commits to the other branches

@michael-o
Copy link
Member

michael-o commented Feb 1, 2021

@martin-g Not really, but I do not fully know which are revelant. I'd rather merge a complete, blessed set rather then some halfbreed.

@martin-g
Copy link
Member

martin-g commented Feb 1, 2021

@michael-o You just pushed two commits to master: https://github.com/apache/tomcat/commits/master:

I'd expect only those two to be backported.

@michael-o
Copy link
Member

@michael-o You just pushed two commits to master: https://github.com/apache/tomcat/commits/master:

* [a616bf3](https://github.com/apache/tomcat/commit/a616bf385a350175a33a0ebf09d8b6688344e9e3) (I guess this is a squashed one)

* [c4f5e2c](https://github.com/apache/tomcat/commit/c4f5e2c9ccd7d65953e3c685d25e436e8330e142)

I'd expect only those two to be backported.

I tried with 9.0.x and compilation fails, precending commits are missing. That is why I requested a backport from @minfrin.

@michael-o
Copy link
Member

@michael-o You just pushed two commits to master: https://github.com/apache/tomcat/commits/master:

* [a616bf3](https://github.com/apache/tomcat/commit/a616bf385a350175a33a0ebf09d8b6688344e9e3) (I guess this is a squashed one)

* [c4f5e2c](https://github.com/apache/tomcat/commit/c4f5e2c9ccd7d65953e3c685d25e436e8330e142)

I'd expect only those two to be backported.

A few fixups will follow, so they need to soak in then someone can cherry pick them to a branch and the squash.

@minfrin
Copy link
Contributor Author

minfrin commented Feb 1, 2021

@minfrin I have now pushed this to master. Can you please backport to 9.0.x and 8.5.x and then I will merge them too?

On the case, thank you.

@minfrin
Copy link
Contributor Author

minfrin commented Feb 1, 2021

@minfrin I have now pushed this to master. Can you please backport to 9.0.x and 8.5.x and then I will merge them too?

Backported to 9.0.x, "ant test-apr" passes.

#402

@minfrin
Copy link
Contributor Author

minfrin commented Feb 2, 2021

Backported to 9.0.x, "ant test-apr" passes.

#402

In trying to backport to 8.5.x, there are some further commits needing backporting to address conflicts:

minfrin@6945b88
minfrin@e99071f
minfrin@1d108f4

@michael-o
Copy link
Member

Yes, let's do 9.0.x first. @rmaucher has added an acceptable solution for Windows. This should be ported to the AprEndpoint too.

@markt-asf
Copy link
Contributor

As far as I can tell, this PR was applied in a616bf3

@markt-asf markt-asf closed this May 19, 2021
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.

5 participants