Decouple Unix from Linux in Native Transport #4335

Scottmitch · 2015-10-09T17:06:00Z

Motivation:
transport-native-epoll is designed to be specific to Linux. However there is native code that can be extracted out and made to work on more Unix like distributions. There are a few steps to be completely decoupled but the first step is to extract out code that can run in a more general Unix environment from the Linux specific code base.

Modifications:

Move all non-Linux specific stuff from Native.java into the io.netty.channel.unix package.
io.netty.channel.unix.FileDescriptor will inherit all the native methods that are specific to file descriptors.
io_netty_channel_epoll_Native.[c|h] will only have code that is specific to Linux.

Result:
Code is decoupled and design is streamlined in FileDescriptor.

Scottmitch · 2015-10-09T17:06:12Z

@normanmaurer @nmittler @trustin - FYI

Scottmitch · 2015-10-09T17:07:18Z

transport-native-epoll/src/main/java/io/netty/channel/epoll/EpollDomainSocketChannel.java

        super(parent, fd.intValue());
    }

    /**
     * Creates a new {@link EpollDomainSocketChannel} from an existing {@link FileDescriptor}
     */
-    public EpollDomainSocketChannel(FileDescriptor fd) {
-        super(fd);
+    public EpollDomainSocketChannel(Socket fd, boolean active) {


@normanmaurer - I spent a bunch of time debugging an issue related to the socket was marked as active because getSoError() returned 0, but then SslHandler.handlerAdded initiated the handshake, which involved doing a write before the FD was actually connected. This results in a rather nebulous channel closed exception which was difficult to track down (the root cause was a 107 - Transport endpoint is not connected as a result of writeAddress). I changed the API here just to discuss ... 1. do you have an example of a time when someone creates an FD and it is already active (as determined by getSoError)? 2. If so I will add another constructor which sets active based upon getSoError.

@normanmaurer - I deprecated all the constructors that appear to be misleading and may get users into trouble.

Scottmitch · 2015-10-15T19:05:39Z

@normanmaurer - PTAL

nmittler · 2015-10-15T20:16:28Z

@Scottmitch took a cursory glance ... looks great! I'll defer to @normanmaurer for a detailed review :)

Scottmitch · 2015-10-15T20:42:37Z

@nmittler - Thanks :) Hopefully paves the way for broader unix support.

normanmaurer · 2015-10-21T09:31:26Z

transport-native-epoll/src/main/c/io_netty_channel_unix_Errors.c

@@ -0,0 +1,191 @@
+/*


@Scottmitch love that this is now in an extra file 👍

normanmaurer · 2015-10-21T09:46:45Z

transport-native-epoll/src/main/java/io/netty/channel/unix/DatagramSocketAddress.java

+    private static final long serialVersionUID = 3094819287843178401L;
+
+    // holds the amount of received bytes
+    final int receivedAmount;


normanmaurer · 2015-10-21T09:50:33Z

@Scottmitch all in all I love this PR! Makes stuff a lot cleaner. Good work, just a few comments buddy!

normanmaurer · 2015-10-23T10:24:54Z

@Scottmitch let us release a new 4.0 and 4.1 before we pull this in so we have a bit more time to test this. WDYT ?

Scottmitch · 2015-10-23T15:40:02Z

@normanmaurer - I think that is a good idea. However I have PR #4336 based off of this PR right now which should prolly go in...

normanmaurer · 2015-10-23T15:42:12Z

@Scottmitch can you maybe refactor #4336 so we can pull it in on the "original" code ?

ninja- · 2015-10-24T17:43:29Z

[INFO] libtool: compile:  gcc -DHAVE_CONFIG_H -I. -I./src -O3 -Werror -I/usr/lib/jvm/java-8-jdk/include -I/usr/lib/jvm/java-8-jdk/include/linux -c src/io_netty_channel_unix_FileDescriptor.c  -fPIC -DPIC -o src/.libs/io_netty_channel_unix_FileDescriptor.o
[INFO] src/io_netty_channel_unix_FileDescriptor.c: In function ‘_write’:
[INFO] src/io_netty_channel_unix_FileDescriptor.c:37:14: error: implicit declaration of function ‘write’ [-Werror=implicit-function-declaration]
[INFO]         res = write(fd, buffer + pos, (size_t) (limit - pos));
[INFO]               ^
[INFO] src/io_netty_channel_unix_FileDescriptor.c: In function ‘_read’:
[INFO] src/io_netty_channel_unix_FileDescriptor.c:65:15: error: implicit declaration of function ‘read’ [-Werror=implicit-function-declaration]
[INFO]          res = read(fd, buffer + pos, (size_t) (limit - pos));
[INFO]                ^
[INFO] src/io_netty_channel_unix_FileDescriptor.c: In function ‘Java_io_netty_channel_unix_FileDescriptor_close’:
[INFO] src/io_netty_channel_unix_FileDescriptor.c:136:8: error: implicit declaration of function ‘close’ [-Werror=implicit-function-declaration]
[INFO]     if (close(fd) < 0) {
[INFO]         ^
[INFO] src/io_netty_channel_unix_FileDescriptor.c: In function ‘Java_io_netty_channel_unix_FileDescriptor_newPipe’:
[INFO] src/io_netty_channel_unix_FileDescriptor.c:224:14: error: implicit declaration of function ‘pipe’ [-Werror=implicit-function-declaration]
[INFO]           if (pipe(fd) == 0) {

this pr broke my build.
need to add

#include <unistd.h>

ninja- · 2015-10-24T17:58:49Z

3 places to fix @Scottmitch and my build process is ok

normanmaurer · 2015-10-25T06:53:19Z

Yeah i guess missing

#include <unistd.h>

Am 24.10.2015 um 19:58 schrieb ninja notifications@github.com:

3 places to fix @Scottmitch and my build process is ok

—
Reply to this email directly or view it on GitHub.

Scottmitch · 2015-10-26T02:01:07Z

@ninja- - Thanks for the comments. The issues you pointed out should be resolved now.

ninja- · 2015-10-29T17:16:24Z

@Scottmitch can your 2 epoll PRs be merged ? I am having a hard time rebasing on them every time

ninja- · 2015-10-29T17:33:36Z

@Scottmitch your pr still has build errors. somehow github wiped my on-line comments when I mentioned fix for the last 3 of them

ninja- · 2015-10-29T17:40:32Z

diff --git a/transport-native-epoll/src/main/c/io_netty_channel_unix_FileDescriptor.c b/transport-native-epoll/src/main/c/io_netty_channel_unix_FileDescriptor.c
index 9ad191c..abcc82b 100644
--- a/transport-native-epoll/src/main/c/io_netty_channel_unix_FileDescriptor.c
+++ b/transport-native-epoll/src/main/c/io_netty_channel_unix_FileDescriptor.c
@@ -17,6 +17,7 @@
 #include <fcntl.h>
 #include <stdlib.h>
 #include <sys/uio.h>
+#include <unistd.h>

 #include "netty_unix_errors.h"
 #include "netty_unix_filedescriptor.h"
diff --git a/transport-native-epoll/src/main/c/io_netty_channel_unix_Socket.c b/transport-native-epoll/src/main/c/io_netty_channel_unix_Socket.c
index de73777..2f39fce 100644
--- a/transport-native-epoll/src/main/c/io_netty_channel_unix_Socket.c
+++ b/transport-native-epoll/src/main/c/io_netty_channel_unix_Socket.c
@@ -23,8 +23,10 @@
 #include <sys/socket.h>
 #include <netinet/in.h>
 #include <netinet/tcp.h>
+#include <arpa/inet.h>

 #include "netty_unix_socket.h"
+#include "netty_unix_errors.h"
 #include "io_netty_channel_unix_Socket.h"

 static jclass datagramSocketAddressClass = NULL;
diff --git a/transport-native-epoll/src/main/c/netty_unix_errors.h b/transport-native-epoll/src/main/c/netty_unix_errors.h
index ba3ad5d..e23f383 100644
--- a/transport-native-epoll/src/main/c/netty_unix_errors.h
+++ b/transport-native-epoll/src/main/c/netty_unix_errors.h
@@ -27,7 +27,7 @@ void netty_unix_errors_throwClosedChannelException(JNIEnv* env);
 void netty_unix_errors_throwOutOfMemoryError(JNIEnv* env);

 // JNI initialization hooks. Users of this file are responsible for calling these in the JNI_OnLoad and JNI_OnUnload methods.
-jint netty_unix_exceptions_JNI_OnLoad(JNIEnv* env);
-void netty_unix_exceptions_JNI_OnUnLoad(JNIEnv* env);
+jint netty_unix_errors_JNI_OnLoad(JNIEnv* env);
+void netty_unix_errors_JNI_OnUnLoad(JNIEnv* env);

 #endif /* NETTY_UNIX_ERRORS_H_ */

normanmaurer · 2015-10-29T17:53:48Z

@Scottmitch @ninja- actually I want to cut a release first (over the next days) and after that merge in as this is a massive change-set and I want to test it a bit more.

Scottmitch · 2015-10-29T18:00:10Z

@ninja- I think I already fixed the issues the first time around (checkout the "Files" in github on this PR) ... maybe you are looking at old code?

ninja- · 2015-10-29T18:05:21Z

@Scottmitch hm ok maybe I forgot to git fetch your branch

normanmaurer · 2015-11-02T20:07:04Z

@Scottmitch just cherry-pick it, so we can move on

Motivation: transport-native-epoll is designed to be specific to Linux. However there is native code that can be extracted out and made to work on more Unix like distributions. There are a few steps to be completely decoupled but the first step is to extract out code that can run in a more general Unix environment from the Linux specific code base. Modifications: - Move all non-Linux specific stuff from Native.java into the io.netty.channel.unix package. - io.netty.channel.unix.FileDescriptor will inherit all the native methods that are specific to file descriptors. - io_netty_channel_epoll_Native.[c|h] will only have code that is specific to Linux. Result: Code is decoupled and design is streamlined in FileDescriptor.

Scottmitch · 2015-11-02T20:59:21Z

Cherry-picked 4.0 (94ef424) 4.1 (dbbdbe1) master (e319a37)

Scottmitch added improvement feature labels Oct 9, 2015

Scottmitch self-assigned this Oct 9, 2015

Scottmitch added this to the 4.0.33.Final milestone Oct 9, 2015

Scottmitch reviewed Oct 9, 2015
View reviewed changes

Scottmitch mentioned this pull request Oct 9, 2015

EPOLL Shutdown and Half Closed #4336

Closed

Scottmitch force-pushed the epoll_unix_decouple branch from 6347e33 to 2eb786c Compare October 9, 2015 17:44

This was referenced Oct 9, 2015

Unix domain sockets coupled to epoll #4231

Closed

Add kqueue channel implementation #2448

Closed

Scottmitch force-pushed the epoll_unix_decouple branch 3 times, most recently from b153586 to a2a6476 Compare October 15, 2015 19:05

Scottmitch force-pushed the epoll_unix_decouple branch from a2a6476 to 9d2212e Compare October 15, 2015 20:48

normanmaurer reviewed Oct 21, 2015
View reviewed changes

Scottmitch force-pushed the epoll_unix_decouple branch from 9d2212e to 381912b Compare October 23, 2015 00:13

Scottmitch force-pushed the epoll_unix_decouple branch from 381912b to 6834851 Compare October 26, 2015 01:59

Scottmitch force-pushed the epoll_unix_decouple branch from 6834851 to 1b9796c Compare October 30, 2015 18:41

Scottmitch force-pushed the epoll_unix_decouple branch from 1b9796c to b7b672e Compare November 2, 2015 20:29

Scottmitch closed this Nov 2, 2015

Scottmitch deleted the epoll_unix_decouple branch November 2, 2015 20:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Decouple Unix from Linux in Native Transport #4335

Decouple Unix from Linux in Native Transport #4335

Scottmitch commented Oct 9, 2015

Scottmitch commented Oct 9, 2015

Scottmitch Oct 9, 2015

Scottmitch Oct 15, 2015

Scottmitch commented Oct 15, 2015

nmittler commented Oct 15, 2015

Scottmitch commented Oct 15, 2015

normanmaurer Oct 21, 2015

Scottmitch Oct 22, 2015

normanmaurer Oct 21, 2015

Scottmitch Oct 23, 2015

normanmaurer commented Oct 21, 2015

normanmaurer commented Oct 23, 2015

Scottmitch commented Oct 23, 2015

normanmaurer commented Oct 23, 2015

ninja- commented Oct 24, 2015

ninja- commented Oct 24, 2015

normanmaurer commented Oct 25, 2015

Scottmitch commented Oct 26, 2015

ninja- commented Oct 29, 2015

ninja- commented Oct 29, 2015

ninja- commented Oct 29, 2015

normanmaurer commented Oct 29, 2015

Scottmitch commented Oct 29, 2015

ninja- commented Oct 29, 2015

normanmaurer commented Nov 2, 2015

Scottmitch commented Nov 2, 2015

Decouple Unix from Linux in Native Transport #4335

Decouple Unix from Linux in Native Transport #4335

Conversation

Scottmitch commented Oct 9, 2015

Scottmitch commented Oct 9, 2015

Scottmitch Oct 9, 2015

Choose a reason for hiding this comment

Scottmitch Oct 15, 2015

Choose a reason for hiding this comment

Scottmitch commented Oct 15, 2015

nmittler commented Oct 15, 2015

Scottmitch commented Oct 15, 2015

normanmaurer Oct 21, 2015

Choose a reason for hiding this comment

Scottmitch Oct 22, 2015

Choose a reason for hiding this comment

normanmaurer Oct 21, 2015

Choose a reason for hiding this comment

Scottmitch Oct 23, 2015

Choose a reason for hiding this comment

normanmaurer commented Oct 21, 2015

normanmaurer commented Oct 23, 2015

Scottmitch commented Oct 23, 2015

normanmaurer commented Oct 23, 2015

ninja- commented Oct 24, 2015

ninja- commented Oct 24, 2015

normanmaurer commented Oct 25, 2015

Scottmitch commented Oct 26, 2015

ninja- commented Oct 29, 2015

ninja- commented Oct 29, 2015

ninja- commented Oct 29, 2015

normanmaurer commented Oct 29, 2015

Scottmitch commented Oct 29, 2015

ninja- commented Oct 29, 2015

normanmaurer commented Nov 2, 2015

Scottmitch commented Nov 2, 2015