[RFC] Netty: add Netty 4 as a new Netty backend and make it default #25
Conversation
DeepCode's analysis on #e858dd found:
Top issues
👉 View analysis in DeepCode’s Dashboard | Configure the bot |
|
Just... WOW! |
|
Just use |
xabolcs
force-pushed
the
branch-netty-4-1-x
branch
from
abc76e2
to
e858dd8
Compare
|
@xabolcs Hi! Thank you for trying to solve the hard problem with upgrading to Netty 4. I have tried your PR and found one problem: serving static resources Namely, I am trying to open the following URL in a browser: and it stays indefinitely in "loading" state. :( |
May I ask a test for it? 😉
That was the problem for Play! 1 too. Let me try to fix it. |
There are |
|
Looks like Hmmm. @tomparle wanted to get rid of
I'll try to just |
|
At (Btw. I inserted |
|
Could a conclusion be added to this issue detailing why the port to netty4 was unsuccessful? This to inform those (who consider) making a similar attempt in the future. I have questions like:
For us dropping the Netty3 dependency would help as we find some incompatibility with libraries (mostly one for Redis) that uses Netty4 in newer versions. We just use an old version now which works fine, so there is no urgency. |
|
@cies as far as I'm concerned :
I cannot tell a lot more since I'm not (yet?) actually contributing to replay, but still on Play1 codebase so that most users can benefit of our contributions. |
|
Hi @cies , as you can see, this is a draft PR with an My attempt failed because my knowledge is limited in context of Netty and RePlay.
I don't know yet, but something looks wrong. The Netty3 -> Netty4 server framework change should be transparent to users of RePlay (e.g. asolntsev / criminals), IMHO! It would be nice if there were end-to-end tests here or in the criminals, to test all the features of RePlay, like "serving static resources".
... is unknown to me. If the Netty3 integration is cleaned up, then this Netty3 -> Netty4 upgrade would be easy. Did I answer your question? |
|
@xabolcs wrote:
Yes, completely. Thanks. |
xabolcs
force-pushed
the
branch-netty-4-1-x
branch
from
e858dd8
to
f8464cd
Compare
xabolcs
force-pushed
the
branch-netty-4-1-x
branch
4 times, most recently
from
907fab9
to
f613acf
Compare
@asolntsev |
xabolcs
changed the title
xabolcs
changed the title
xabolcs
force-pushed
the
branch-netty-4-1-x
branch
from
f613acf
to
87c0de2
Compare
This comment was marked as off-topic.
This comment was marked as off-topic.
xabolcs
force-pushed
the
branch-netty-4-1-x
branch
from
87c0de2
to
479f2a1
Compare
I'm going to split up the changes to multiple commits:
There are a lot of duplicated code, because of package only visibility. Should I change them to
If my upgrade attempt is stable enough, I would like add Netty 5 too on another branch, but on top of this. ... To make this Netty3 -> Netty 4 upgrade more compact and robust. |
Thanks @asolntsev for finding that haystack! That checkbox is already checked, so I hope you could add more commit to my branch: |
How about splitting commit "Netty: add Netty 4 as a new Netty backend and make it default" to just add Netty 4, but only making default after your fixes? Just checkout my branch, rebase, add your changes and force push to my branch, or just open a new PR with those rewritten commits. |
Sounds good. I will try to do that. |
Thank you for the hint! |
xabolcs
force-pushed
the
branch-netty-4-1-x
branch
from
9fc4d94
to
bb06d20
Compare
So there are two problem at least! 😀
|
No relevant code in the trace 😕 |
🤔 Some related SO: |
Yup, but before migrating to Netty 4! if (writeFuture != null) {
writeFuture.addListener(future -> logger.trace("served {} in {} ms", file, formatNanos(nanoTime() - startedAt)));... and do the |
|
@xabolcs Yes, it seems the problem with ignore |
@asolntsev |
This is an important test as it's failing with Netty 4. See pull request #25.
* users need to add dependency either `replay-netty3` or `replay-netty4` * each module has its own `play.server.Server` class * replay own integration tests run on Netty4 implementation details: * class `NettyInvocation` was not really needed anymore; now we just extend `Invocation`. * I had to find a replacement for `Server.httpPort` which was only used in PDF module. I hope `request.getBase()` is as good as `"http://localhost:" + Server.httpPort`. :)
* users need to add dependency either `replay-netty3` or `replay-netty4` * each module has its own `play.server.Server` class * replay own integration tests run on Netty4 implementation details: * class `NettyInvocation` was not really needed anymore; now we just extend `Invocation`. * I had to find a replacement for `Server.httpPort` which was only used in PDF module. I hope `request.getBase()` is as good as `"http://localhost:" + Server.httpPort`. :)
* users need to add dependency either `replay-netty3` or `replay-netty4` * each module has its own `play.server.Server` class * replay own integration tests run on Netty4 implementation details: * class `NettyInvocation` was not really needed anymore; now we just extend `Invocation`. * I had to find a replacement for `Server.httpPort` which was only used in PDF module. I hope `request.getBase()` is as good as `"http://localhost:" + Server.httpPort`. :)
* users need to add dependency either `replay-netty3` or `replay-netty4` * each module has its own `play.server.Server` class * replay own integration tests run on Netty4 implementation details: * class `NettyInvocation` was not really needed anymore; now we just extend `Invocation`. * I had to find a replacement for `Server.httpPort` which was only used in PDF module. I hope `request.getBase()` is as good as `"http://localhost:" + Server.httpPort`. :)
|
@xabolcs @tomparle @cies Hi.
UPD Here is the pull request for |
You already opened a PR for that, back in January - #115 🙂 |
|
@xabolcs Ah yes, that's right. :) Thank you. :) |
|
@cies @xabolcs @tomparle Hi.
Feel free to share you feedback. |
|
Nice work ! |
|
@tomparle Thank you! But I am pretty sure that in real projects (like internet-banks etc.), the asynchrony of Netty was not really used. So the only case where asynchrony of Netty could have some benefit is serving static resources. But nowadays static resources are usually served by a separate http server (e.g. Ngynx, Amazon or other cloud service) and cached by browser. So I don't think there should be some remarkable difference in performance. |
For Play! 1 @tomparle did a great work already. It's still work in progress, but it's a great milestone.
I tried my luck for Play! 1, based on his work, but failed too. 😅
Now I tried my luck for RePlay because it's simpler than Play! 1 ... and it looks good. 👍
Ideas, thoughts:
ByteRangeandByteRangeInput(andFileService?) could be simplified IMHOPlayHandler.LazyChunkedInputalso needs more love (Raw use of parameterized classChunkedInput)PlayHandler:FullHttpRequestFullHttpResponsepublic class PlayHandler<I extends FullHttpRequest, O extends FullHttpResponse> extends SimpleChannelInboundHandler<I> { ... }PlayHandleris too big ... it would be nice to separate the Netty handler parts from the Play! internalsI didn't tested this in production
Therefore this is just a draft.
Tests were green, both here and at asolntsev / criminals.