b instanceof FileChannelBuffer: never happened in my projects 🤷

[xabolcs]

I'm currently rebasing #25 to master which recently got 4ea9553 as a fix for #41.

Because I wasn't able to port FileChannelBuffer to Netty4 I commented out the if (b instanceof FileChannelBuffer) { ... } section and kept the else { ... } path of the code.

All tests were still green so I didn't care much! 😀

But @cies came, and reported issue #41. 🙂

@cies, may I ask you to add a reproducer example project in the replay-tests folder?
It would be nice if it would have (a test which) exercise the b instanceof FileChannelBuffer codepath in PlayHandler.parseRequest()!

Thanks!

[cies]

Hi @xabolcs, sorry for the horribly late reply.

When I reported #41 I did not know what call exactly caused the problem. I expect it was a file upload, but many file uploads when perfectly fine. I just found the error in the logs, confirmed it was a new error never seen with Play1, and went looking for a fix by studying the code. The problem was easily identified and several ways to fix it were discussed, then Andrei came up with an even better way to fix it, released it, and that completely removed the issue off our radar.

So I dont know what was special about the (presumed) file uploads that caused the problem...

I did a search on the warning that is now logged by resetBodyInputStreamIfPossible in framework/src/play/data/parsing/TextParser.java:

log.warn("Failed to reset request.body of type {}: {}",
                resetNotSupported.getClass().getName(), resetNotSupported.toString());

This warning occurs between 15 to 50x per day in our logs in form of:

parsing.TextParser Failed to reset request.body of type java.io.FileNotFoundException: java.io.FileNotFoundException: /home/stager/tmp/0ad52251-299b-472f-87a1-ac5ee3335f2d (No such file or directory)

Digging deeper I found they relate to a single endpoint (controller method), the only method that contains the following (suspicious) code:

request.params.get("body");

I can try to create a test case that triggers the warning, if that helps?

Or any other thing that I can do to help?

[xabolcs]

Hi xabolcs, sorry for the horribly late reply.

No problem as Netty 4 isn't ready yet! 🙃

In #41 Andrei wrote:

@cies Yes, Replay calls method request.body.reset() which can only work if request.body is ByteArrayInputStream. And it is in most cases (because it's initialized in PlayHandler.parseRequest()) except the case when b instanceof FileChannelBuffer. Which never happened in my projects.

Well, we probably should remove request.body.reset(), though I don't know what can go wrong in our projects. This reset was added for some reason, see 91cae9f

So I thought your warnings came from the app because your app excercised those b instanceof FileChannelBuffer lines of the code.

Of course, I could be wrong! 😀

I can try to create a test case that triggers the warning, if that helps?

Or any other thing that I can do to help?

As I wrote in the issue description:
"It would be nice if it would have (a test which) exercise the b instanceof FileChannelBuffer codepath in PlayHandler.parseRequest()!" 🙏

[asolntsev]

@xabolcs What you mean "Netty 4 isn't ready yet"? Netty 4.1.79.Final exists for years. Did you mean that Netty 5 is not ready yet?

[xabolcs]

What you mean "Netty 4 isn't ready yet"?

My Netty upgrade isn't ready yet: PR #25

[cies]

So I thought your warnings came from the app because your app exercised those b instanceof FileChannelBuffer lines of the code.

It was a bit of a search to find the cause of the parsing.TextParser Failed to reset request.body of type java.io.FileNotFoundException warnings, I had to add some instrumentation to a production build to find out what caused them as all my initial attempts to recreate them in a RePlay test case failed.

But I got it! It is due to RePlay receiving a large body (with a content-length larger than ~10000), then the type of request.body changes from ByteArrayInputStream to ResettableFileInputStream and the warning in thrown. ResettableFileInputStream is wrapped by FileChannelBuffer.

So I managed to create a uitest that exercises those b instanceof FileChannelBuffer lines of the code and also logs the mentioned warning.

It's a bit of a clumsy test, as it uses uitest to get there. Also I do not know if this helps you out @xabolcs , so please let me know if it helps or what else you need/prefer.

[xabolcs]

It's a bit of a clumsy test, as it uses uitest to get there. Also I do not know if this helps you out @xabolcs , so please let me know if it helps or what else you need/prefer.

Thank you very much!

It surely will help, as #25 now have two new tests to pass! The other one (static files) is failing already with Netty 4. 😀

[cies]

@xabolcs Most welcome. And thank you for working on one of biggest concerns on our upgrade path. :)

If there's any other way I can support you, please let me know.