-
Notifications
You must be signed in to change notification settings - Fork 3.4k
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
[WIP] Implement Dropwizard Unix socket connector #1893
Conversation
3525675
to
b577d32
Compare
Regarding the second error I believe we affected by jetty/jetty.project#1044.
|
@JsonTypeName("unixsocket") | ||
public class UnixSocketFactory extends HttpConnectorFactory { | ||
|
||
private String socketFilename = "/tmp/dropwizard.sock"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we prefer /var/run/dropwizard.sock
(Stackoverflow)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a good idea.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After the second thought, it looks not so good. Not every process has enough permissions to write to the /var/run
directory. I believe, you need to have a special directory under /var/run
with the permissions for the user which runs the process.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah that's fine. If I were using unix sockets I'd most likely change the socket location anyways...to something like /var/run/<servicename>.sock
. But it may be something to add in the documentation (eg. normally unix domain sockets are in /var/run
, but since we may not be running as root or have adequate permissions, store the file in /tmp
)
b577d32
to
b30c295
Compare
import java.io.Reader; | ||
import java.nio.channels.Channels; | ||
|
||
public class UnixSocketConnectorFactoryTest { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
May need to disable this test for windows builds, no?
* </tr> | ||
* <tr> | ||
* <td>{@code socketFilename}</td> | ||
* <td></tmp/dropwizard.sock</td> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there's an extra <
in here
jetty-9.4.1.v20170120 is out and fixes the timeout issue, though it doesn't fix the bug where socket errors arise under load |
b30c295
to
1ef2b68
Compare
f0f336f
to
31598aa
Compare
I think the bug in the jnr library hasn't been fixed. I believe I submitted a PR for a while: jnr/jnr-enxio#25 |
bfd004b
to
6e7bfd9
Compare
|
||
@Before | ||
public void setUp() { | ||
assumeFalse(SystemUtils.IS_OS_WINDOWS); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does anyone have an idea why this doesn't seem to work on AppVeyor?
https://ci.appveyor.com/project/arteam/dropwizard/build/1836#L10337
[ERROR] testManyClientCalls(io.dropwizard.unixsocket.UnixSocketConnectorFactoryTest) Time elapsed: 1.795 s <<< ERROR!
java.lang.UnsatisfiedLinkError:
The operation completed successfully.
[ERROR] testClient(io.dropwizard.unixsocket.UnixSocketConnectorFactoryTest) Time elapsed: 0 s <<< ERROR!
java.lang.UnsatisfiedLinkError:
The operation completed successfully.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By UnsatisfiedLinkError
looks like that native method not found.
Maybe you have to specify image to run maven in appveyor, e.g. image: ubuntu
Here more info:
https://www.appveyor.com/docs/getting-started-with-appveyor-for-linux/#running-your-build-on-linux
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@alex-shpak The whole point of using AppVeyor is to make sure that Dropwizard can be built on Windows. 😉
From my understanding, the assumption in the @Before
method should prevent all tests from running on Windows. But that doesn't seem to work. 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Try:
@BeforeClass
public static void setUp() {
assumeFalse(SystemUtils.IS_OS_WINDOWS);
}
Edit: (that made the test ignored on my windows machine)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nickbabcock Seems to have worked. Thanks!
https://ci.appveyor.com/project/arteam/dropwizard/build/1838#L8658
@arteam @joschi @nickbabcock this all set? tests passed 👍 💯 🎉 |
@jplock I'd like to wait for the pull request in jnr/jnr-enxio to be merged until we merge this PR. Additionally, the tests in But at least the branch now builds on all of our CI systems. 😉 |
It looks like Jetty fixed their integration issues with jnr, and I don't see 30 seconds timeouts during shutdown. I don't know anything about socket errors under load, haven't done any performance testing. |
Don't worry, I did the load test in the jetty issue: jetty/jetty.project#1281 and no errors arose. |
Is this going to be merged in? It would be awesome to have this feature in |
@MadalinaPatrichi We currently don't plan to push this further. If you're interested in this feature, it would be great if you could help us work out the problems with this PR. |
What are the current problems? Travis builds and no assertions in |
dd9dfd4
to
682895d
Compare
@natnan Yes, pretty much the missing test coverage, the flaky tests, and some transitive dependency foobar. |
I'm a bit hesitant to invest more time into this, given that it's not a stable feature in Jetty. jetty/jetty.project#3136 (comment)
|
@dropwizard/committers Anyone wants to chime in? Otherwise I'd close this PR unless it's working reliably and the UNIX socket support in Jetty becomes stable. |
Yeah an relying on an experimental feature would be shaky. I'm in favor of closing and re-evaluating in the future. |
Closed due to experimental feature in upstream project. |
hey there. this can potentially be revived now considering it's not experimental anymore: https://webtide.com/unixdomain-support-in-jetty/ |
@natan-abolafya Thanks for looking into this. Would you like to create a pull request against the |
Will be very happy to try. I should be able to look into it probably early next week. |
Co-authored-by: Steffen Nießing <zuniquex@protonmail.com> ###### Problem: Since Java 16, Unix Domain Sockets are supported natively. And since Jetty 10, there is a ready made connector for it and it is not considered experimental anymore. The typical use case is a reverse proxy connecting to UNIX socket instead of TCP which has various advantages. ###### Solution: Created a new connector type `unix-socket`. I've reimplemented #1893 but it needed some adjustments considering how much Dropwizard changed since then. The test was also a little tricky (and not very pretty) considering there is no UNIX socket support in Apache HTTP client. Or at least I couldn't find any but please correct me otherwise. ###### Result: It's pretty simple to listen on unix sockets now. That was something we wanted in our product for a long time. Also this works on Windows since Windows now also supports UNIX domain sockets. Refs #1893
Co-authored-by: Steffen Nießing <zuniquex@protonmail.com> Since Java 16, Unix Domain Sockets are supported natively. And since Jetty 10, there is a ready made connector for it and it is not considered experimental anymore. The typical use case is a reverse proxy connecting to UNIX socket instead of TCP which has various advantages. Created a new connector type `unix-socket`. I've reimplemented #1893 but it needed some adjustments considering how much Dropwizard changed since then. The test was also a little tricky (and not very pretty) considering there is no UNIX socket support in Apache HTTP client. Or at least I couldn't find any but please correct me otherwise. It's pretty simple to listen on UNIX sockets now. That was something we wanted in our product for a long time. Also this works on Windows since Windows now also supports UNIX domain sockets. Refs #1893 Refs #7614 (cherry picked from commit 1de3eef)
Co-authored-by: Steffen Nießing <zuniquex@protonmail.com> Since Java 16, Unix Domain Sockets are supported natively. And since Jetty 10, there is a ready made connector for it and it is not considered experimental anymore. The typical use case is a reverse proxy connecting to UNIX socket instead of TCP which has various advantages. Created a new connector type `unix-socket`. I've reimplemented #1893 but it needed some adjustments considering how much Dropwizard changed since then. The test was also a little tricky (and not very pretty) considering there is no UNIX socket support in Apache HTTP client. Or at least I couldn't find any but please correct me otherwise. It's pretty simple to listen on UNIX sockets now. That was something we wanted in our product for a long time. Also this works on Windows since Windows now also supports UNIX domain sockets. Refs #1893 Refs #7614 (cherry picked from commit 1de3eef)
Co-authored-by: Steffen Nießing <zuniquex@protonmail.com> Since Java 16, Unix Domain Sockets are supported natively. And since Jetty 10, there is a ready made connector for it and it is not considered experimental anymore. The typical use case is a reverse proxy connecting to UNIX socket instead of TCP which has various advantages. Created a new connector type `unix-socket`. I've reimplemented #1893 but it needed some adjustments considering how much Dropwizard changed since then. The test was also a little tricky (and not very pretty) considering there is no UNIX socket support in Apache HTTP client. Or at least I couldn't find any but please correct me otherwise. It's pretty simple to listen on UNIX sockets now. That was something we wanted in our product for a long time. Also this works on Windows since Windows now also supports UNIX domain sockets. Refs #1893 Refs #7614 (cherry picked from commit 1de3eef)
Co-authored-by: Steffen Nießing <zuniquex@protonmail.com> Since Java 16, Unix Domain Sockets are supported natively. And since Jetty 10, there is a ready made connector for it and it is not considered experimental anymore. The typical use case is a reverse proxy connecting to UNIX socket instead of TCP which has various advantages. Created a new connector type `unix-socket`. I've reimplemented #1893 but it needed some adjustments considering how much Dropwizard changed since then. The test was also a little tricky (and not very pretty) considering there is no UNIX socket support in Apache HTTP client. Or at least I couldn't find any but please correct me otherwise. It's pretty simple to listen on UNIX sockets now. That was something we wanted in our product for a long time. Also this works on Windows since Windows now also supports UNIX domain sockets. Refs #1893 Refs #7614 (cherry picked from commit 1de3eef)
Co-authored-by: Steffen Nießing <zuniquex@protonmail.com> Since Java 16, Unix Domain Sockets are supported natively. And since Jetty 10, there is a ready made connector for it and it is not considered experimental anymore. The typical use case is a reverse proxy connecting to UNIX socket instead of TCP which has various advantages. Created a new connector type `unix-socket`. I've reimplemented #1893 but it needed some adjustments considering how much Dropwizard changed since then. The test was also a little tricky (and not very pretty) considering there is no UNIX socket support in Apache HTTP client. Or at least I couldn't find any but please correct me otherwise. It's pretty simple to listen on UNIX sockets now. That was something we wanted in our product for a long time. Also this works on Windows since Windows now also supports UNIX domain sockets. Refs #1893 Refs #7614 (cherry picked from commit 1de3eef)
Co-authored-by: Steffen Nießing <zuniquex@protonmail.com> Since Java 16, Unix Domain Sockets are supported natively. And since Jetty 10, there is a ready made connector for it and it is not considered experimental anymore. The typical use case is a reverse proxy connecting to UNIX socket instead of TCP which has various advantages. Created a new connector type `unix-socket`. I've reimplemented #1893 but it needed some adjustments considering how much Dropwizard changed since then. The test was also a little tricky (and not very pretty) considering there is no UNIX socket support in Apache HTTP client. Or at least I couldn't find any but please correct me otherwise. It's pretty simple to listen on UNIX sockets now. That was something we wanted in our product for a long time. Also this works on Windows since Windows now also supports UNIX domain sockets. Refs #1893 Refs #7614 (cherry picked from commit 1de3eef)
Co-authored-by: Steffen Nießing <zuniquex@protonmail.com> Co-authored-by: Natan Abolafya <natan.abolafya@appgate.com> Since Java 16, Unix Domain Sockets are supported natively. And since Jetty 10, there is a ready made connector for it and it is not considered experimental anymore. The typical use case is a reverse proxy connecting to UNIX socket instead of TCP which has various advantages. Created a new connector type `unix-socket`. I've reimplemented #1893 but it needed some adjustments considering how much Dropwizard changed since then. The test was also a little tricky (and not very pretty) considering there is no UNIX socket support in Apache HTTP client. Or at least I couldn't find any but please correct me otherwise. It's pretty simple to listen on UNIX sockets now. That was something we wanted in our product for a long time. Also this works on Windows since Windows now also supports UNIX domain sockets. Refs #1893 Refs #7614 (cherry picked from commit 1de3eef)
Co-authored-by: Steffen Nießing <zuniquex@protonmail.com> Co-authored-by: Natan Abolafya <natan.abolafya@appgate.com> Since Java 16, Unix Domain Sockets are supported natively. And since Jetty 10, there is a ready made connector for it and it is not considered experimental anymore. The typical use case is a reverse proxy connecting to UNIX socket instead of TCP which has various advantages. Created a new connector type `unix-socket`. I've reimplemented #1893 but it needed some adjustments considering how much Dropwizard changed since then. The test was also a little tricky (and not very pretty) considering there is no UNIX socket support in Apache HTTP client. Or at least I couldn't find any but please correct me otherwise. It's pretty simple to listen on UNIX sockets now. That was something we wanted in our product for a long time. Also this works on Windows since Windows now also supports UNIX domain sockets. Refs #1893 Refs #7614 (cherry picked from commit 1de3eef)
This pull requests some basic work for supporting a Unix Socket connector in Dropwizard. The implementation seems to be rather simple, but I stumled on some problems:
The original issue is #1889.