Add nio http server transport #29587
Conversation
|
This is probably ready for a review. There are a few tests I want to add, but the http server work should be complete. It does not support CORs or pipeling. But that will be added in future PRs. There is a decent amount of code duplication between netty4 module and nio, but that can be addressed later. |
|
I wlll look @tbrooks8 thank for opening it. |
There was a problem hiding this comment.
I am surprised how small it is. Yet, still pretty big for a real review. I left some nits and questions here and there but nothing in-depth. I don't think it's possible to be honest. Are there parts you feel I should pay more attention to, you know where the complexity is.
| synchronized (serverChannels) { | ||
| if (!serverChannels.isEmpty()) { | ||
| // TODO: Wait | ||
| for (NioServerSocketChannel serverChannel : serverChannels) { |
| } | ||
|
|
||
| // TODO: Wait | ||
| for (NioSocketChannel channel : socketChannels) { |
| // ignore | ||
| return; | ||
| } | ||
| if (!NetworkExceptionHelper.isCloseConnectionException(cause)) { |
There was a problem hiding this comment.
can we use == false across the board?
| * @param listener the listener to be called when the message is sent | ||
| * @return the write operation to be queued | ||
| */ | ||
| WriteOperation createWriteOperation(SocketChannelContext context, Object message, BiConsumer<Void, Throwable> listener); |
There was a problem hiding this comment.
any chance we can make this typesafe?
There was a problem hiding this comment.
This is something I have thought about and I don't think it is very easy to do in this PR. The ReadWriteHandler would need to be parameterized. And then the channel context would need to be parameterized. And then the channel.
I think down the line it might be more possible. With implementing an "http" channel type that is a subclass of a niochannel (similar to how we have a tcp channel type). But I think that would be a lot more code and better to do as follow-up work.
The |
There was a problem hiding this comment.
@tbrooks8 I did another pass and looked at the classes you pointed out. I also looked at the permissions this gets and glanced at the tests. I am very happy with the level of testing that happens here. Good job. I am all for pushing this in and get some benchmarks going as well.
|
|
||
| @Override | ||
| public WriteOperation createWriteOperation(SocketChannelContext context, Object message, BiConsumer<Void, Throwable> listener) { | ||
| if (message instanceof FullHttpResponse) { |
There was a problem hiding this comment.
should this be an assertion?
| @Override | ||
| public void close() throws IOException { | ||
| try { | ||
| adaptor.close(); |
There was a problem hiding this comment.
oh boy what is netty doing here again :D
| releaseContent = false; | ||
| releaseBytesStreamOutput = false; | ||
| } finally { | ||
| if (releaseContent) { |
There was a problem hiding this comment.
these can all throw exceptions no? I think what can happen here is that 1. we might miss closing the second one and 2. we might hide / miss an exception? I wonder if we can refactor this method a bit and use a List also for these two. Then we can just pass it to IOUtils#close and if we don't wanna release it we just clear the list?
There was a problem hiding this comment.
also do we need to call nioChannel.close() here too?
| @Override | ||
| public Method method() { | ||
| HttpMethod httpMethod = request.method(); | ||
| if (httpMethod == HttpMethod.GET) |
There was a problem hiding this comment.
maybe use a switch statement here?
| grant codeBase "${codebase.netty-transport}" { | ||
| // Netty NioEventLoop wants to change this, because of https://bugs.openjdk.java.net/browse/JDK-6427854 | ||
| // the bug says it only happened rarely, and that its fixed, but apparently it still happens rarely! | ||
| permission java.util.PropertyPermission "sun.nio.ch.bugLevel", "write"; |
There was a problem hiding this comment.
LOL oh boy. I thought we don't need it and it will just log a warning from the netty end?
|
|
||
| // This should only currently be required as we use the netty http client | ||
| // netty makes and accepts socket connections | ||
| permission java.net.SocketPermission "*", "accept,connect"; |
There was a problem hiding this comment.
can you elaborate here, why do we use the http client?
There was a problem hiding this comment.
we might only grant this to tests?
|
|
||
| grant codeBase "${codebase.netty-common}" { | ||
| // for reading the system-wide configuration for the backlog of established sockets | ||
| permission java.io.FilePermission "/proc/sys/net/core/somaxconn", "read"; |
There was a problem hiding this comment.
this also looks like it's unneeded since netty catches the exception. Maybe we just omit it?
This commit is related to #28898. It adds an nio driven http server
transport. Currently it only supports basic http features. Cors,
pipeling, and read timeouts will need to be added in future PRs.