javaee / grizzly

Writing scalable server applications in the Java™ programming language has always been difficult. Before the advent of the Java New I/O API (NIO), thread management issues made it impossible for a server to scale to thousands of users. The Grizzly NIO framework has been designed to help developers to take advantage of the Java™ NIO API.
https://javaee.github.io/grizzly/
Other
222 stars 60 forks source link

OutputBuffer interleaves output when processing characters and bytes #1839

Closed glassfishrobot closed 8 years ago

glassfishrobot commented 8 years ago

Writing to OutputBuffer has two ways of write() (byte[] and char[]/String) wrote to different buffers in Grizzly, and when flushed, the buffers were concatenated after-the-fact, thus causing corrupted output.

This is especially evident in processing JSP files in GlassFish 4.x

// pseudo-code out.print('aaa')
out.print((convert-to byte[]) 'bbb')
out.print('ccc')
out.print((convert-to byte[]) 'ddd')

This produces 'bbbdddaaaccc' instead of expected 'aaabbbcccddd'

Environment

All

Affected Versions

[2.3.24]

glassfishrobot commented 8 years ago

Reported by lprimak

glassfishrobot commented 8 years ago

oleksiys said: If you use standard servlet API - it won't let you use OutputStream and Writer for the same response. OutputBuffer is low-level writer, that has to be used very carefully, if what you see happens for JSP files - it's definitely a bug in Glassfish.

glassfishrobot commented 8 years ago

lprimak said: I know it's a bug, but I disagree about the cause. GlassFish just uses Grizzly as a back-end, and just calls it's API as you see above. It's not documented anywhere that you can't mix write(byte[]) with write(String/char[]) in Grizzly, especially since it corrupts output.

Are you saying that it's illegal to mix the two write() variants in OutputBuffer?

glassfishrobot commented 8 years ago

oleksiys said: OutputBuffer shouldn't be used normally by external users, because in servlet module you use response.getWriter or getOutputStream, similar for Grizzly HttpHandler. If you for whatever reason decide to use low level OutputBuffer - you become responsible for flushing char[] or byte[] buffers yourself, as appropriate for your usecase.

glassfishrobot commented 8 years ago

lprimak said: Ok, let me start over.

I discovered a bug in the Glassfish 4 -> Grizzly integration that affects my application. After days and days of debugging, I figured out that the way JSPs executed leads to interleaved Grizzly calls to OutputBuffer.write(char[]/String) and OutputBuffer.write(byte[]), which in turn leads to corrupted output.

The question is how to resolve this issue.

There are two possible causes:

So, there are two possible ways to resolve this issue: 1- Fix Grizzly to support interleaved calls to all variations of write() correctly 2- Fix GlassFish to only call one variation of OutputBuffer.write() and do charset conversion in GF as opposed to let Grizzly handle it.

Or, there is another solution I am not seeing here.

How should I proceed?

Thank you!

glassfishrobot commented 8 years ago

oleksiys said: IMO Glassfish has to be fixed, because OutputBuffer doesn't really have to care about char[] and byte[] buffer ordering. If you give me a jsp or better servlet to reproduce this issue - I can check Glassfish code.

glassfishrobot commented 8 years ago

lprimak said: So, as to confirm, Grizzy doesn't allow for interleaved calls to OutputBuffer.write(char[]) and OutputBuffer.write(byte[]) correct? If yes, this should be documented.

In that case, indeed the GF code needs to be fixed just to use OutputBufer.write(byte[]) and use it's own charset conversion. I have debugged this extensively and this is indeed what the issue is. As a matter of fact, I did fix GF code already in development and just wondering if this is a valid solution of is Grizzly has to be fixed.

Thanks for your help.

glassfishrobot commented 8 years ago

ondrejm said: @oleksiys, thanks for looking into this. Would you please check this JSP and generated java code (generated by Jasper inside GF), what's wrong with it? The generated code uses org.apache.jasper.runtime.JspWriterImpl to write to the response. I also checked the code of JspWriterImpl and it seems to be using only Writer and no OutputStream. I don't see how it could possibly mix OutputStream and Writer, therefore I don't think that the issue comes from mixing the two together. Can you see another problem with the generated JSP code?

glassfishrobot commented 8 years ago

@rlubke said: I believe due to the fact that your JSP isn't buffered, a feature of the JSP engine is being enabled. I'd like to see if explicitly disabling this feature resolves the issue. Can you edit the $GF_HOME/domains//config/default-web.xml, and add the following init parameter to the JspServlet:

<init-param>
  <param-name>genStrAsByteArray</param-name>
  <param-value>false</param-value>
</init-param>
glassfishrobot commented 8 years ago

lprimak said: Ryan that's correct. If you disable unbuffered JSP output the interleaved grizzly OutputBuffer calls don't happen and thus the problem doesn't manifest itself. However this is not an appropriate solution to the problem for most GassFish and Payara users.

The root cause is still either a bug in Grizzly, or undocumented API side effect of calling write() with byte[] or char[] on the same OutputBuffer

glassfishrobot commented 8 years ago

@rlubke said: Root of the issue is that OutputBuffer doesn't have a mode where buffering is disabled.

Working through the scope of what would need to be changed to support this.

While I understand having no Buffer is desirable, for now, I think disabling the genStrAsByteArray option is a fair workaround for now.

glassfishrobot commented 8 years ago

lprimak said: I have already made a patch to Payara to fix the issue, but I think that the underlying Grizzly issue is the unexpected behavior when interleaving calls to OutputBuffer.write() with char[] and byte[]

This Grizzly behavior needs to be documented or, better yet, an exception should be thrown if both variations of write() are used. I can think of another way to fix it, without a need for throwing the exceptions instead of using two separate buffers for write(char[]) and write(byte[]) only one buffer could be used, and when using write(char[]) and variants, it would do a char to byte conversion and use the single byte buffer.

glassfishrobot commented 8 years ago

lprimak said: I guess this leads to implementation question: Why are there 3 buffers in OutputBuffer? char buffer, byte buffer and composite buffer? Couldn't this be just one buffer and every call would write into that one?

glassfishrobot commented 8 years ago

@rlubke said: The byte buffer is self explanatory. The char buffer is an optimization as the characters being buffered could be held within a single char buffer, but could result in the byte buffer being flushed multiple times. The composite buffer is to avoid byte copies.

glassfishrobot commented 8 years ago

lprimak said: Why not just convert chars on the spot and put them into ByteBuffer? Seems easier to me and avoids this issue. This is what was done in previous Grizzly versions.

glassfishrobot commented 8 years ago

@rlubke said: Changes applied.

2.3.x: be8a9c395219e11dbf46c862874b9363bddbd322 master: 59b020e975c48851e1d7717039ee8b6ff6462011

glassfishrobot commented 8 years ago

@rlubke said: It's all performance related. We had a simpler approach and ended up having a performance regression. These changes improved the performance characteristics.

glassfishrobot commented 8 years ago

lprimak said: Looking good, with tests! thanks!

glassfishrobot commented 7 years ago

This issue was imported from java.net JIRA GRIZZLY-1839

glassfishrobot commented 8 years ago

Marked as fixed on Thursday, June 30th 2016, 1:56:48 pm