-
-
Notifications
You must be signed in to change notification settings - Fork 15.9k
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
New native transport for kqueue #6000
Conversation
@normanmaurer @nmittler @marcuslinke @timothyjward @shpikat @caarlos0 @ninja- @rnorth @ndeloof @slandelle @headius (everyone on the related issues) - PTAL. There is a large amount of code here ... sorry about that. However it would be nice to get some miles on it and gain more confidence and uncover any issues ASAP. There is also some duplication in the Channel hierarchy between EPOLL and KQUEUE. It is not possible for them to share a common hierarchy so for now I think its is OK to keep the duplication and look to improve/consolidate later. |
🎉 🎉 🎉 I'll give it a try as soon as I can (will probably be next week). Thanks a lot! |
Build failure seems to be related to not being able to find the artifact from a new sub-module introduced in this PR. I recall seeing this issue in the past on the CI servers, and thought we changed the build order or added an extra build pass to resolve this type of issues ... @trustin @normanmaurer do you recall?
|
nice to see that ! |
@normanmaurer @trustin - Looks like we do the following on the build servers:
can we instead do:
|
@Scottmitch Sounds great! Will try it this weekend. Thanks for your effort! |
@Scottmitch we not want to do an "install" as this will install "changes" that may break other builds on the same host. I wonder if we can't tell maven to just resolve this somehow internally or if we can't tell maven to use an temporary local maven repository. |
Let me investigate |
@Scottmitch ok seems like I can use a custom local maven repos... Let me try this out on the CI and kick off another build. Also thanks for this "massive" work :) |
@Scottmitch for the record the build is successful locally here :) |
@Scottmitch ok it should now work on the CI as soon as the build is fixed for linux. The problem is it builds and installs this one: But it looks for: The unix-common should not add Please fix and the build should work :) |
@normanmaurer - I actually want the classifier on the artifact ... but I was just inconsistently using However it looks like the build server is still doing the |
@Scottmitch let me check the config again... maybe I missed to change it to install but at least it uses the local repository now. Why you need to use the "-fedora" classifier ? We not used it before in the epoll transport as it works on fedora and debian etc.... |
Not specifically the "-fedora" type classifier ... just the same classifier used by the epoll module (e.g. |
When trying to bootstrap my application like this:
I get an exception
Maybe have declared the wrong dependencies ?
|
You also need to specify the qualifier for the unix-common dependency AFAIC
|
@normanmaurer Using classifier 'osx-x86_64' I get the same exception. |
@marcuslinke - While its in PR form make sure you do the following:
And your Let me know if there are still issues after trying this. This is how the
|
@Scottmitch The problem is that I've to support both systems (linux and macos), so how this should work. As I understand the |
@Scottmitch Why |
Depends how you want to achieve this.
Perhaps but is their a reasons to hardcode and not just use the recommended approach? |
|
@Scottmitch Your branch compiles and installs fine here. But even when I do as suggested I get the same exception... java.lang.UnsatisfiedLinkError: failed to load the required native library
|
@Scottmitch You said the native library |
I cloned the PR and got Unix Domain Sockets running on my Mac via kqueue! Thank you guys for your great work :) I ran into an issue during shutdown though. I'm not sure if it is because I am using Netty incorrectly, please forgive my ignorance if that is the case. Our shutdown logic for a Unix domain socket server is:
where This same logic worked for Epoll running on Linux. But when I tried the above with kqueue, it hangs indefinitely on the |
7885fb8
to
e21dab3
Compare
@dannyferreira - Can you try and reverse the order in which you shutdown the event loop and close the channel? The channel may need to use the event loop for shutdown operations. |
build failure unrelated and captured in issue #6577 |
@Scottmitch - closing the channel before shutting down the event loop seems to work well for both kqueue and epoll. Thanks! |
@normanmaurer - Any objections to merging this? I'm not sure that anyone has actually reviewed the code. We need to coordinate the release cycle and builds on the CI servers. Release CycleI'm not very familiar with how the maven release process works but IIRC we need to do something like the following:
It doesn't matter which platform is built first but the CI server PR buildsWe need to build PRs on MacOS and Linux (to some degree). It would be nice if we could just build the |
|
||
try { | ||
boolean done = false; | ||
for (int i = config().getWriteSpinCount() - 1; i >= 0; i--) { |
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.
We might avoid subtraction -1
for the any loops with *SpinCount
(where i
don't use inside),
if change i >= 0
to i > 0
:
for (int i = config().getWriteSpinCount(); i > 0; i--)
(But this is only a matter of style.)
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.
sgtm
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.
This applies not only to the specified line. Some other loops in this PR are iterated by writeSpinCount
. To be consistent, it would be nice to change them too :)
@Scottmitch please see the comment: Scottmitch#1 (comment) |
189714e
to
4708cb3
Compare
@Scottmitch Managed to build and run your kqueue transport and run it with docker java but seems it fails on one of my colleagues machine (he didn't build your branch but just reused the jar): dyld: lazy symbol binding failed: Symbol not found: _netty_unix_util_parse_package_prefix Any idea ? |
eb3c860
to
15b46ab
Compare
Motivation: We currently don't have a native transport which supports kqueue https://www.freebsd.org/cgi/man.cgi?query=kqueue&sektion=2. This can be useful for BSD systems such as MacOS to take advantage of native features, and provide feature parity with the Linux native transport. Modifications: - Make a new transport-native-unix-common module with all the java classes and JNI code for generic unix items. This module will build a static library for each unix platform, and included in the dynamic libraries used for JNI (e.g. transport-native-epoll, and eventually kqueue). - Make a new transport-native-unix-common-tests module where the tests for the transport-native-unix-common module will live. This is so each unix platform can inherit from these test and ensure they pass. - Add a new transport-native-kqueue module which uses JNI to directly interact with kqueue Result: JNI support for kqueue. Fixes netty#2448 Fixes netty#4231
4.1 (3cc4052) |
@lucboutier - What version of macOS was the artifact built on, and what version of macOS are you trying to use the artifact on? Now that the PR has been merged can you open an issue? |
@Scottmitch Tried to use kqueue native transport under MacOS 10.11.6 and got
Any idea if this could be fixed somehow? |
Created an issue for it: #6837 |
Motivation:
We currently don't have a native transport which supports kqueue https://www.freebsd.org/cgi/man.cgi?query=kqueue&sektion=2. This can be useful for BSD systems such as MacOS to take advantage of native features, and provide feature parity with the Linux native transport.
Modifications:
Result:
JNI support for kqueue.
Fixes #2448
Fixes #4231