Skip to content
GitHub no longer supports this web browser. Learn more about the browsers we support.
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 when using native epoll transport #3344

Closed
wants to merge 1 commit into from

Conversation

@normanmaurer
Copy link
Member

normanmaurer commented Jan 21, 2015

Motivation:

Using Unix Domain Sockets can be very useful when communication should take place on the same host and has less overhead then using loopback. We should support this with the native epoll transport.

Modifications:

  • Add support for Unix Domain Sockets.
  • Adjust testsuite to be able to reuse tests.

Result:

Unix Domain Sockets are now support when using native epoll transport.

@normanmaurer

This comment has been minimized.

Copy link
Member Author

normanmaurer commented Jan 21, 2015

@trustin please review

if((ctrl_msg->cmsg_level == SOL_SOCKET) && (ctrl_msg->cmsg_type == SCM_RIGHTS)) {
socketFd = *((int *) CMSG_DATA(ctrl_msg));
// set as non blocking as we want to use it with epoll
if (fcntl(socketFd, F_SETFL, O_NONBLOCK) == -1) {

This comment has been minimized.

Copy link
@Scottmitch

Scottmitch Jan 21, 2015

Member

Do we have a consensus on ioctl vs fcntl (i.e. always use fcntl for portability/standardization reasons)? Word on the street is ioctl can sometimes be less expensive for this case.

This comment has been minimized.

Copy link
@normanmaurer

normanmaurer Jan 21, 2015

Author Member

Nope... can you tell me more about these "word on the street" ? Maybe links etc...

This comment has been minimized.

Copy link
@Scottmitch

Scottmitch Jan 21, 2015

Member

@normanmaurer - Sure. For example see nginx comment at the top.

The "word on the street" expression is because I didn't have anything concrete but was also interested if you had any feed back (been outstanding question of mine, and haven't dug into the kernel to investigate).

This comment has been minimized.

Copy link
@normanmaurer

normanmaurer Jan 22, 2015

Author Member

That's interesting... did not know this. I guess we should use ioctl then :)

This comment has been minimized.

This comment has been minimized.

Copy link
@Scottmitch

Scottmitch Jan 22, 2015

Member

Yes I also looked at that stackoverflow post. The comments in nginx were more convincing to me (listing the calls that fcntl does). I think for setting non-blocking ioctl is the way to go, but unfortunately I'm not sure if ioctl is always better.

This comment has been minimized.

Copy link
@normanmaurer

normanmaurer Jan 26, 2015

Author Member

Let me open another issue for that. I would love to investigate a bit more here before doing any changes that may affect things in a bad way.

This comment has been minimized.

Copy link
@normanmaurer
*/
private boolean writeBytes(ChannelOutboundBuffer in, ByteBuf buf) throws Exception {
int readableBytes = buf.readableBytes();
if (readableBytes == 0) {

This comment has been minimized.

Copy link
@Scottmitch

Scottmitch Jan 21, 2015

Member

Consider using isReadable() for clarity.

This comment has been minimized.

Copy link
@normanmaurer

normanmaurer Jan 21, 2015

Author Member

I used readableBytes as we use the returned value later . So we can safe one method invocation

This comment has been minimized.

Copy link
@Scottmitch

Scottmitch Jan 21, 2015

Member

Sounds reasonable. I remember using this rational for JdkZlibDecoder (and JZlibDecoder) but I don't think @trustin liked it.

This comment has been minimized.

Copy link
@trustin

trustin Jan 26, 2015

Member

I prefer reducing unnecessary method calls as you expected. :-)

This comment has been minimized.

Copy link
@Scottmitch

Scottmitch Jan 26, 2015

Member

Sounds good. I'll submit a PR to do the same for the decoders (small issue but before I forget I may as well just clean it up).

return true;
}

if (buf.hasMemoryAddress() || buf.nioBufferCount() == 1) {

This comment has been minimized.

Copy link
@Scottmitch

Scottmitch Jan 21, 2015

Member

Plug for #3306 :)

}
} else {
ByteBuffer nioBuf;
if (buf.nioBufferCount() == 1) {

This comment has been minimized.

Copy link
@Scottmitch

Scottmitch Jan 21, 2015

Member

Another plug for #3306 :)

break;
}
try {
readPending = false;

This comment has been minimized.

Copy link
@Scottmitch

Scottmitch Jan 21, 2015

Member

Would it be a problem to move this outside of try { .. }? Just in the interest of minimizing exception block.

This comment has been minimized.

Copy link
@normanmaurer

normanmaurer Jan 21, 2015

Author Member

you mean the readPending = true ?

This comment has been minimized.

Copy link
@Scottmitch

Scottmitch Jan 21, 2015

Member

Yes. This is a nit of course, so treat it accordingly.

This comment has been minimized.

Copy link
@normanmaurer

normanmaurer Jan 22, 2015

Author Member

Sure... that said I guess it makes no difference

This comment has been minimized.

Copy link
@Scottmitch

Scottmitch Jan 22, 2015

Member

Agreed. Very minor, but more just good practice to reduce the amount of code in the try {..} scope.

This comment has been minimized.

Copy link
@normanmaurer

normanmaurer Jan 26, 2015

Author Member

agreed

@NiteshKant

This comment has been minimized.

Copy link
Member

NiteshKant commented Jan 21, 2015

@normanmaurer Nice! This is required in netflix, so its great that you got it going!

@normanmaurer

This comment has been minimized.

Copy link
Member Author

normanmaurer commented Jan 21, 2015

@NiteshKant BOOM ! ;)

break;
}
expectedWrittenBytes -= localWrittenBytes;
writtenBytes += localWrittenBytes;

This comment has been minimized.

Copy link
@Scottmitch

Scottmitch Jan 21, 2015

Member

To avoid redundant computation would it make sense to compute this once at the end? For example:

final long initialExpectedWrittenBytes = expectedWrittenBytes;

for (;;) { /*do writes and decrement expectedWrittenBytes*/ }

in.removeBytes(initialExpectedWrittenBytes - expectedWrittenBytes);

This comment has been minimized.

Copy link
@normanmaurer

normanmaurer Jan 22, 2015

Author Member

I guess yeah we could do this. Not sure it makes any difference though

This comment has been minimized.

Copy link
@Scottmitch

Scottmitch Jan 22, 2015

Member

Just less computation and memory access required. A reduction from O(n) load/add/store operations (where n is the number of iterations required for this loop) to O(1).

This comment has been minimized.

Copy link
@trustin

This comment has been minimized.

Copy link
@normanmaurer

normanmaurer Jan 26, 2015

Author Member

@Scottmitch done... I guess we should do the same in nio from which this code derived :)

break;
}
expectedWrittenBytes -= localWrittenBytes;
writtenBytes += localWrittenBytes;

This comment has been minimized.

Copy link
@Scottmitch

This comment has been minimized.

Copy link
@normanmaurer

normanmaurer Jan 26, 2015

Author Member

done

if (buf.nioBufferCount() == 1) {
nioBuf = buf.internalNioBuffer(buf.readerIndex(), buf.readableBytes());
} else {
nioBuf = buf.nioBuffer();

This comment has been minimized.

Copy link
@Scottmitch

Scottmitch Jan 21, 2015

Member

Has adding something like adding a forEach(ByteBufferProcess processor) method to the ByteBuf interface been discussed recently? Where ByteBufferProcess would have a method which iterates over all the "internal" ByteBuffer objects. The goal of this interface would be to prevent having to always combine everything into 1 ByteBuffer all the time.

Some additional complexities of this approach may include:

  • checking if each ByteBuffer is direct or not, and then copying into direct to do native writes if necessary.
  • supporting a index and length may make the interface a bit wider.

This comment has been minimized.

Copy link
@normanmaurer

normanmaurer Jan 22, 2015

Author Member

Not sure we really need this... let me think about it a bit.

This comment has been minimized.

Copy link
@Scottmitch

Scottmitch Jan 23, 2015

Member

Ok I can open another issue if necessary. Seems like our code which uses byte buffers is often fragmented into conditionals based upon internal buffer count. I'm wondering if this could alleviate the special conditionals and make the usage a bit cleaner. I think most ByteBuf implementations could have the fast path which does no looping (because there is only 1 buffer).

This comment has been minimized.

Copy link
@trustin

trustin Jan 26, 2015

Member

Don't we already have ByteBuf.forEachByte()?

This comment has been minimized.

Copy link
@trustin

trustin Jan 26, 2015

Member

Ugh, nevermind. :-)

This comment has been minimized.

Copy link
@trustin

trustin Jan 26, 2015

Member

Currently, the only ByteBuf implementation that can have more than one ByteBuffer is CompositeByteBuf, so I think it's probably better adding a method like forEachComponent() to CompositeByteBuf only.

This comment has been minimized.

Copy link
@trustin

trustin Jan 26, 2015

Member

and yeah, please feel free to file an issue. I think it might be useful.

This comment has been minimized.

Copy link
@normanmaurer

normanmaurer Jan 26, 2015

Author Member

Yep... but let us handle this a separate issue and merge this first.

This comment has been minimized.

Copy link
@normanmaurer

normanmaurer Jan 26, 2015

Author Member

@Scottmitch please open an issue to track this. Thanks!

This comment has been minimized.

Copy link
@Scottmitch
@normanmaurer normanmaurer self-assigned this Jan 22, 2015
return fd;
}

JNIEXPORT jint JNICALL Java_io_netty_channel_epoll_Native_bindDomainSocket(JNIEnv * env, jclass clazz, jint fd, jstring socketPath) {

This comment has been minimized.

Copy link
@trustin

trustin Jan 26, 2015

Member

I see:

  • JNIEnv * env,
  • JNIEnv *env, and
  • JNIEnv* env

Could you use JNIEnv* env everywhere?

This comment has been minimized.

Copy link
@trustin

trustin Jan 26, 2015

Member

Same for other pointer declarations, such as const char*

This comment has been minimized.

Copy link
@normanmaurer

normanmaurer Jan 26, 2015

Author Member

done

memset(&addr, 0, sizeof(addr));
addr.sun_family = AF_UNIX;

const char *socket_path = (*env)->GetStringUTFChars(env, socketPath, 0);

This comment has been minimized.

Copy link
@trustin

trustin Jan 26, 2015

Member

I wonder if we should convert this to the current system character encoding, because not all systems use UTF-8 as the system encoding.

This comment has been minimized.

Copy link
@normanmaurer

normanmaurer Jan 26, 2015

Author Member

you mean by convert them to byte[] in java and pass the byte[] to the jni function ?

@@ -49,6 +50,10 @@
this.active = active;
}

protected int fd() {

This comment has been minimized.

Copy link
@trustin

trustin Jan 26, 2015

Member

final?

This comment has been minimized.

Copy link
@normanmaurer

normanmaurer Jan 26, 2015

Author Member

yep

}

public DomainSocketAddress(File file) {
this(file.getAbsolutePath());

This comment has been minimized.

Copy link
@trustin

trustin Jan 26, 2015

Member
  1. Could you explain why you convert file to absolute path?
  2. When file is null, NPE("file") should be raised.

This comment has been minimized.

Copy link
@normanmaurer

normanmaurer Jan 26, 2015

Author Member

Why you think absolute path is not correct ? Also a NPE will be raised by the JVM as I do file.get*...

This comment has been minimized.

Copy link
@trustin

trustin Feb 4, 2015

Member

I'm not saying it's incorrect. Just curious. :-) Is there a reason not to use file.toString() instead?
The NPE raised by file.get..() has no message, although it's subtle.

This comment has been minimized.

Copy link
@normanmaurer

normanmaurer Feb 4, 2015

Author Member

I think it is correct yeah :) So all good and ready to merge ?

This comment has been minimized.

Copy link
@trustin

trustin Feb 4, 2015

Member

Nope :-)
Is there a reason that you used getAbsolutePath() instead of getPath()? What benefit does getAbsolutePath() give us really?

This comment has been minimized.

Copy link
@trustin

trustin Feb 4, 2015

Member

The thing is, getAbsolutePath() never guarantees that the returned path string is unique. You'll have to use getCanonicalPath() to get it, but it's expensive and it throws an exception. So, unless there's a good reason, I would use getPath() for simplicity here.

This comment has been minimized.

Copy link
@normanmaurer

normanmaurer Feb 4, 2015

Author Member

ha... I think you are right... getPath() is better. Let me fix this. Anything else ?

This comment has been minimized.

Copy link
@normanmaurer

normanmaurer Feb 4, 2015

Author Member

yep.. will replace with getPath(). Thanks for pushing it :)

This comment has been minimized.

Copy link
@trustin

trustin Feb 4, 2015

Member

All good now. Thanks for your patience! Please merge after fixing this.

/**
* The path to the domain socket.
*/
public String socketPath() {

This comment has been minimized.

Copy link
@trustin

trustin Jan 26, 2015

Member

How about just path for brevity?

This comment has been minimized.

Copy link
@normanmaurer

normanmaurer Jan 26, 2015

Author Member

sure why not.

File socketFile = new File(local.socketPath());
boolean success = socketFile.delete();
if (!success && logger.isDebugEnabled()) {
logger.debug(String.format("Failed to delete domain socket file %s", local.socketPath()));

This comment has been minimized.

Copy link
@trustin

trustin Jan 26, 2015

Member

You should do this instead:

logger.debug("Failed to delete a domain socket file: {}", local.socketPath());

This comment has been minimized.

Copy link
@normanmaurer

normanmaurer Jan 26, 2015

Author Member

done


EpollSocketChannel(Channel parent, int fd) {
super(parent, fd, Native.EPOLLIN, true);
public EpollSocketChannel(Channel parent, int fd) {

This comment has been minimized.

Copy link
@trustin

trustin Jan 26, 2015

Member

Is it intentional to add public here? Just curious.

This comment has been minimized.

Copy link
@normanmaurer

normanmaurer Jan 26, 2015

Author Member

nope... good catch.

int res = bindDomainSocket(fd, addr.socketPath());
if (res < 0) {
throw newIOException("bind", res);
}
}

This comment has been minimized.

Copy link
@trustin

trustin Jan 26, 2015

Member

Raise an error if neither cases are met.

This comment has been minimized.

Copy link
@normanmaurer

normanmaurer Jan 26, 2015

Author Member

done

} else {
DomainSocketAddress unixDomainSocketAddress = (DomainSocketAddress) socketAddress;
res = connectDomainSocket(fd, unixDomainSocketAddress.socketPath());
}

This comment has been minimized.

Copy link
@trustin

trustin Jan 26, 2015

Member

Same - raise an error when neither cases are met

This comment has been minimized.

Copy link
@normanmaurer

normanmaurer Jan 26, 2015

Author Member

done

}

public static DomainSocketAddress newSocketAddress() {
return new DomainSocketAddress("/tmp/netty.dsocket");

This comment has been minimized.

Copy link
@trustin

trustin Jan 26, 2015

Member

Could you use a real temporary file name? Otherwise we will see failures when we run two builds in the same machine.

This comment has been minimized.

Copy link
@normanmaurer

normanmaurer Jan 26, 2015

Author Member

Good point

@trustin

This comment has been minimized.

Copy link
Member

trustin commented Jan 26, 2015

First pass done. Please ping me once ready for another pass.

@normanmaurer

This comment has been minimized.

Copy link
Member Author

normanmaurer commented Jan 26, 2015

@trustin ping :)

Motivation:

Using Unix Domain Sockets can be very useful when communication should take place on the same host and has less overhead then using loopback. We should support this with the native epoll transport.

Modifications:

- Add support for Unix Domain Sockets.
- Adjust testsuite to be able to reuse tests.

Result:

Unix Domain Sockets are now support when using native epoll transport.
@normanmaurer normanmaurer force-pushed the domain_socket_1 branch from 217e176 to 5f2be6a Jan 29, 2015
@normanmaurer

This comment has been minimized.

Copy link
Member Author

normanmaurer commented Feb 4, 2015

Cherry-picked into 4.0 (b898bdd), 4.1 (f771a97) and master (19ec0f3)

@normanmaurer normanmaurer deleted the domain_socket_1 branch Feb 6, 2015
@mrniko mrniko mentioned this pull request Jun 19, 2015
@lingfengsan lingfengsan mentioned this pull request May 31, 2018
@zuoqinbo

This comment has been minimized.

Copy link

zuoqinbo commented Sep 4, 2019

i want to got a link for Unix domain Socket guide demo ,use netty!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.