You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@tomcat.apache.org by rj...@apache.org on 2013/12/17 22:43:02 UTC
svn commit: r1551729 -
/tomcat/tc7.0.x/trunk/test/org/apache/catalina/websocket/TestWebSocket.java
Author: rjung
Date: Tue Dec 17 21:43:02 2013
New Revision: 1551729
URL: http://svn.apache.org/r1551729
Log:
Fix occasional test failure.
The WebSocketClient first read headers via
a BufferedReader, then tried to read the body
via the underlying InputStream. Depending on
the structure of the incoming packets reading
the body failed because some bytes were already
buffered in the reader and no longer available
by the stream. The switch between rader and stream
was motivated, because the decoding also had to
switch from ISO-8859-1 (headers) to UTF-8 (body).
We now simulate a rudimentary reader which always
reads from the stream and allows to change the
decoding charset while reading.
Modified:
tomcat/tc7.0.x/trunk/test/org/apache/catalina/websocket/TestWebSocket.java
Modified: tomcat/tc7.0.x/trunk/test/org/apache/catalina/websocket/TestWebSocket.java
URL: http://svn.apache.org/viewvc/tomcat/tc7.0.x/trunk/test/org/apache/catalina/websocket/TestWebSocket.java?rev=1551729&r1=1551728&r2=1551729&view=diff
==============================================================================
--- tomcat/tc7.0.x/trunk/test/org/apache/catalina/websocket/TestWebSocket.java (original)
+++ tomcat/tc7.0.x/trunk/test/org/apache/catalina/websocket/TestWebSocket.java Tue Dec 17 21:43:02 2013
@@ -16,13 +16,11 @@
*/
package org.apache.catalina.websocket;
-import java.io.BufferedReader;
+import java.io.BufferedInputStream;
import java.io.IOException;
import java.io.InputStream;
-import java.io.InputStreamReader;
import java.io.OutputStream;
import java.io.OutputStreamWriter;
-import java.io.Reader;
import java.io.Writer;
import java.net.InetSocketAddress;
import java.net.Socket;
@@ -382,12 +380,11 @@ public class TestWebSocket extends Tomca
private static class WebSocketClient {
private OutputStream os;
- private InputStream is;
private boolean isContinuation = false;
final String encoding = "ISO-8859-1";
- private Socket socket ;
- private Writer writer ;
- private BufferedReader reader;
+ private Socket socket;
+ private Writer writer;
+ private CustomReader reader;
public WebSocketClient(int port) {
SocketAddress addr = new InetSocketAddress("localhost", port);
@@ -397,9 +394,7 @@ public class TestWebSocket extends Tomca
socket.connect(addr, 10000);
os = socket.getOutputStream();
writer = new OutputStreamWriter(os, encoding);
- is = socket.getInputStream();
- Reader r = new InputStreamReader(is, encoding);
- reader = new BufferedReader(r);
+ reader = new CustomReader(socket.getInputStream(), encoding);
} catch (Exception e) {
throw new RuntimeException(e);
}
@@ -449,28 +444,100 @@ public class TestWebSocket extends Tomca
}
private String readMessage() throws IOException {
- ByteChunk bc = new ByteChunk(125);
- CharChunk cc = new CharChunk(125);
// Skip first byte
- is.read();
+ reader.read();
// Get payload length
- int len = is.read() & 0x7F;
+ int len = reader.read() & 0x7F;
assertTrue(len < 126);
// Read payload
- int read = 0;
- while (read < len) {
- read = read + is.read(bc.getBytes(), read, len - read);
+ reader.setEncoding("UTF-8");
+ return reader.read(len);
+ }
+ /*
+ * This is not a real reader but just a thin wrapper around
+ * an input stream that allows to switch encoding during
+ * reading.
+ * An example is reading headers using ISO-8859-1 and a body
+ * using UTF-8.
+ * The class is not thread-safe and not well-performing.
+ */
+ private class CustomReader {
+ private InputStream is;
+ private String encoding;
+ private boolean markSupported;
+ private B2CConverter b2c;
+
+ public CustomReader(InputStream is, String encoding) throws IOException {
+ this.is = new BufferedInputStream(is);
+ this.encoding = encoding;
+ markSupported = is.markSupported();
+ b2c = new B2CConverter(encoding);
+ }
+
+ public String getEncoding() {
+ return encoding;
+ }
+
+ public void setEncoding(String encoding) throws IOException {
+ this.encoding = encoding;
+ b2c = new B2CConverter(encoding);
}
- bc.setEnd(len);
+ public int read() throws IOException {
+ return is.read();
+ }
- B2CConverter b2c = new B2CConverter("UTF-8");
- b2c.convert(bc, cc, true);
+ public String read(int len) throws IOException {
+ ByteChunk bc = new ByteChunk(125);
+ CharChunk cc = new CharChunk(125);
+ int read = 0;
+ while (read < len) {
+ read = read + is.read(bc.getBytes(), read, len - read);
+ }
+
+ bc.setEnd(len);
+ b2c.convert(bc, cc, true);
+ return cc.toString();
+ }
- return cc.toString();
+ public String readLine() throws IOException {
+ ByteChunk bc = new ByteChunk(125);
+ CharChunk cc = new CharChunk(125);
+ char c;
+ int i = is.read();
+ int read = 0;
+ while (i != -1) {
+ if (i != '\r' && i != '\n') {
+ bc.append((byte)i);
+ read++;
+ } else if (i == '\n') {
+ break;
+ } else if (i == '\r') {
+ if (markSupported) {
+ is.mark(2);
+ }
+ i = read();
+ if (i == -1) {
+ break;
+ } else {
+ if (i == '\n') {
+ break;
+ } else {
+ if (markSupported) {
+ is.reset();
+ }
+ }
+ }
+ }
+ i = is.read();
+ }
+ bc.setEnd(read);
+ b2c.convert(bc, cc, true);
+ return cc.toString();
+ }
}
}
}
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: svn commit: r1551729 - /tomcat/tc7.0.x/trunk/test/org/apache/catalina/websocket/TestWebSocket.java
Posted by Rainer Jung <ra...@kippdata.de>.
On 17.12.2013 23:06, sebb wrote:
> On 17 December 2013 21:43, <rj...@apache.org> wrote:
>> Author: rjung
>> Date: Tue Dec 17 21:43:02 2013
>> New Revision: 1551729
>>
>> URL: http://svn.apache.org/r1551729
>> Log:
>> Fix occasional test failure.
>>
>> The WebSocketClient first read headers via
>> a BufferedReader, then tried to read the body
>> via the underlying InputStream. Depending on
>> the structure of the incoming packets reading
>> the body failed because some bytes were already
>> buffered in the reader and no longer available
>> by the stream. The switch between rader and stream
>> was motivated, because the decoding also had to
>> switch from ISO-8859-1 (headers) to UTF-8 (body).
>>
>> We now simulate a rudimentary reader which always
>> reads from the stream and allows to change the
>> decoding charset while reading.
>
> It would be helpful to include this log message in the code comments.
>
> The commit log is basically ephemeral - it should (only) document why
> the commit was made.
> In this case "Fix occasional test failure." would be sufficient.
>
> Comments that may be needed to understand why the code behaves in a
> particular way belong as comments in the code.
> After all, the mission of the ASF is to release source, which should
> be able to stand on its own.
That's why I added the comment
+ /*
+ * This is not a real reader but just a thin wrapper around
+ * an input stream that allows to switch encoding during
+ * reading.
+ * An example is reading headers using ISO-8859-1 and a body
+ * using UTF-8.
+ * The class is not thread-safe and not well-performing.
+ */
to the class. IMHO here it doesn't make sense to add an explanation for
the old no longer existing bug. IMHO it is a general anti-pattern not
especially related to this test class so it doesn't make sense to
document any past bugs in the code.
Regards,
Rainer
>> Modified:
>> tomcat/tc7.0.x/trunk/test/org/apache/catalina/websocket/TestWebSocket.java
>>
>> Modified: tomcat/tc7.0.x/trunk/test/org/apache/catalina/websocket/TestWebSocket.java
>> URL: http://svn.apache.org/viewvc/tomcat/tc7.0.x/trunk/test/org/apache/catalina/websocket/TestWebSocket.java?rev=1551729&r1=1551728&r2=1551729&view=diff
>> ==============================================================================
>> --- tomcat/tc7.0.x/trunk/test/org/apache/catalina/websocket/TestWebSocket.java (original)
>> +++ tomcat/tc7.0.x/trunk/test/org/apache/catalina/websocket/TestWebSocket.java Tue Dec 17 21:43:02 2013
>> @@ -16,13 +16,11 @@
>> */
>> package org.apache.catalina.websocket;
>>
>> -import java.io.BufferedReader;
>> +import java.io.BufferedInputStream;
>> import java.io.IOException;
>> import java.io.InputStream;
>> -import java.io.InputStreamReader;
>> import java.io.OutputStream;
>> import java.io.OutputStreamWriter;
>> -import java.io.Reader;
>> import java.io.Writer;
>> import java.net.InetSocketAddress;
>> import java.net.Socket;
>> @@ -382,12 +380,11 @@ public class TestWebSocket extends Tomca
>>
>> private static class WebSocketClient {
>> private OutputStream os;
>> - private InputStream is;
>> private boolean isContinuation = false;
>> final String encoding = "ISO-8859-1";
>> - private Socket socket ;
>> - private Writer writer ;
>> - private BufferedReader reader;
>> + private Socket socket;
>> + private Writer writer;
>> + private CustomReader reader;
>>
>> public WebSocketClient(int port) {
>> SocketAddress addr = new InetSocketAddress("localhost", port);
>> @@ -397,9 +394,7 @@ public class TestWebSocket extends Tomca
>> socket.connect(addr, 10000);
>> os = socket.getOutputStream();
>> writer = new OutputStreamWriter(os, encoding);
>> - is = socket.getInputStream();
>> - Reader r = new InputStreamReader(is, encoding);
>> - reader = new BufferedReader(r);
>> + reader = new CustomReader(socket.getInputStream(), encoding);
>> } catch (Exception e) {
>> throw new RuntimeException(e);
>> }
>> @@ -449,28 +444,100 @@ public class TestWebSocket extends Tomca
>> }
>>
>> private String readMessage() throws IOException {
>> - ByteChunk bc = new ByteChunk(125);
>> - CharChunk cc = new CharChunk(125);
>>
>> // Skip first byte
>> - is.read();
>> + reader.read();
>>
>> // Get payload length
>> - int len = is.read() & 0x7F;
>> + int len = reader.read() & 0x7F;
>> assertTrue(len < 126);
>>
>> // Read payload
>> - int read = 0;
>> - while (read < len) {
>> - read = read + is.read(bc.getBytes(), read, len - read);
>> + reader.setEncoding("UTF-8");
>> + return reader.read(len);
>> + }
>> + /*
>> + * This is not a real reader but just a thin wrapper around
>> + * an input stream that allows to switch encoding during
>> + * reading.
>> + * An example is reading headers using ISO-8859-1 and a body
>> + * using UTF-8.
>> + * The class is not thread-safe and not well-performing.
>> + */
>> + private class CustomReader {
>> + private InputStream is;
>> + private String encoding;
>> + private boolean markSupported;
>> + private B2CConverter b2c;
>> +
>> + public CustomReader(InputStream is, String encoding) throws IOException {
>> + this.is = new BufferedInputStream(is);
>> this.encoding = encoding;
>> + markSupported = is.markSupported();
>> + b2c = new B2CConverter(encoding);
>> + }
>> +
>> + public String getEncoding() {
>> + return encoding;
>> + }
>> +
>> + public void setEncoding(String encoding) throws IOException {
>> + this.encoding = encoding;
>> + b2c = new B2CConverter(encoding);
>> }
>>
>> - bc.setEnd(len);
>> + public int read() throws IOException {
>> + return is.read();
>> + }
>>
>> - B2CConverter b2c = new B2CConverter("UTF-8");
>> - b2c.convert(bc, cc, true);
>> + public String read(int len) throws IOException {
>> + ByteChunk bc = new ByteChunk(125);
>> + CharChunk cc = new CharChunk(125);
>> + int read = 0;
>> + while (read < len) {
>> + read = read + is.read(bc.getBytes(), read, len - read);
>> + }
>> +
>> + bc.setEnd(len);
>> + b2c.convert(bc, cc, true);
>> + return cc.toString();
>> + }
>>
>> - return cc.toString();
>> + public String readLine() throws IOException {
>> + ByteChunk bc = new ByteChunk(125);
>> + CharChunk cc = new CharChunk(125);
>> + char c;
>> + int i = is.read();
>> + int read = 0;
>> + while (i != -1) {
>> + if (i != '\r' && i != '\n') {
>> + bc.append((byte)i);
>> + read++;
>> + } else if (i == '\n') {
>> + break;
>> + } else if (i == '\r') {
>> + if (markSupported) {
>> + is.mark(2);
>> + }
>> + i = read();
>> + if (i == -1) {
>> + break;
>> + } else {
>> + if (i == '\n') {
>> + break;
>> + } else {
>> + if (markSupported) {
>> + is.reset();
>> + }
>> + }
>> + }
>> + }
>> + i = is.read();
>> + }
>> + bc.setEnd(read);
>> + b2c.convert(bc, cc, true);
>> + return cc.toString();
>> + }
>> }
>> }
>> }
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: svn commit: r1551729 - /tomcat/tc7.0.x/trunk/test/org/apache/catalina/websocket/TestWebSocket.java
Posted by sebb <se...@gmail.com>.
On 17 December 2013 21:43, <rj...@apache.org> wrote:
> Author: rjung
> Date: Tue Dec 17 21:43:02 2013
> New Revision: 1551729
>
> URL: http://svn.apache.org/r1551729
> Log:
> Fix occasional test failure.
>
> The WebSocketClient first read headers via
> a BufferedReader, then tried to read the body
> via the underlying InputStream. Depending on
> the structure of the incoming packets reading
> the body failed because some bytes were already
> buffered in the reader and no longer available
> by the stream. The switch between rader and stream
> was motivated, because the decoding also had to
> switch from ISO-8859-1 (headers) to UTF-8 (body).
>
> We now simulate a rudimentary reader which always
> reads from the stream and allows to change the
> decoding charset while reading.
It would be helpful to include this log message in the code comments.
The commit log is basically ephemeral - it should (only) document why
the commit was made.
In this case "Fix occasional test failure." would be sufficient.
Comments that may be needed to understand why the code behaves in a
particular way belong as comments in the code.
After all, the mission of the ASF is to release source, which should
be able to stand on its own.
> Modified:
> tomcat/tc7.0.x/trunk/test/org/apache/catalina/websocket/TestWebSocket.java
>
> Modified: tomcat/tc7.0.x/trunk/test/org/apache/catalina/websocket/TestWebSocket.java
> URL: http://svn.apache.org/viewvc/tomcat/tc7.0.x/trunk/test/org/apache/catalina/websocket/TestWebSocket.java?rev=1551729&r1=1551728&r2=1551729&view=diff
> ==============================================================================
> --- tomcat/tc7.0.x/trunk/test/org/apache/catalina/websocket/TestWebSocket.java (original)
> +++ tomcat/tc7.0.x/trunk/test/org/apache/catalina/websocket/TestWebSocket.java Tue Dec 17 21:43:02 2013
> @@ -16,13 +16,11 @@
> */
> package org.apache.catalina.websocket;
>
> -import java.io.BufferedReader;
> +import java.io.BufferedInputStream;
> import java.io.IOException;
> import java.io.InputStream;
> -import java.io.InputStreamReader;
> import java.io.OutputStream;
> import java.io.OutputStreamWriter;
> -import java.io.Reader;
> import java.io.Writer;
> import java.net.InetSocketAddress;
> import java.net.Socket;
> @@ -382,12 +380,11 @@ public class TestWebSocket extends Tomca
>
> private static class WebSocketClient {
> private OutputStream os;
> - private InputStream is;
> private boolean isContinuation = false;
> final String encoding = "ISO-8859-1";
> - private Socket socket ;
> - private Writer writer ;
> - private BufferedReader reader;
> + private Socket socket;
> + private Writer writer;
> + private CustomReader reader;
>
> public WebSocketClient(int port) {
> SocketAddress addr = new InetSocketAddress("localhost", port);
> @@ -397,9 +394,7 @@ public class TestWebSocket extends Tomca
> socket.connect(addr, 10000);
> os = socket.getOutputStream();
> writer = new OutputStreamWriter(os, encoding);
> - is = socket.getInputStream();
> - Reader r = new InputStreamReader(is, encoding);
> - reader = new BufferedReader(r);
> + reader = new CustomReader(socket.getInputStream(), encoding);
> } catch (Exception e) {
> throw new RuntimeException(e);
> }
> @@ -449,28 +444,100 @@ public class TestWebSocket extends Tomca
> }
>
> private String readMessage() throws IOException {
> - ByteChunk bc = new ByteChunk(125);
> - CharChunk cc = new CharChunk(125);
>
> // Skip first byte
> - is.read();
> + reader.read();
>
> // Get payload length
> - int len = is.read() & 0x7F;
> + int len = reader.read() & 0x7F;
> assertTrue(len < 126);
>
> // Read payload
> - int read = 0;
> - while (read < len) {
> - read = read + is.read(bc.getBytes(), read, len - read);
> + reader.setEncoding("UTF-8");
> + return reader.read(len);
> + }
> + /*
> + * This is not a real reader but just a thin wrapper around
> + * an input stream that allows to switch encoding during
> + * reading.
> + * An example is reading headers using ISO-8859-1 and a body
> + * using UTF-8.
> + * The class is not thread-safe and not well-performing.
> + */
> + private class CustomReader {
> + private InputStream is;
> + private String encoding;
> + private boolean markSupported;
> + private B2CConverter b2c;
> +
> + public CustomReader(InputStream is, String encoding) throws IOException {
> + this.is = new BufferedInputStream(is);
> + this.encoding = encoding;
> + markSupported = is.markSupported();
> + b2c = new B2CConverter(encoding);
> + }
> +
> + public String getEncoding() {
> + return encoding;
> + }
> +
> + public void setEncoding(String encoding) throws IOException {
> + this.encoding = encoding;
> + b2c = new B2CConverter(encoding);
> }
>
> - bc.setEnd(len);
> + public int read() throws IOException {
> + return is.read();
> + }
>
> - B2CConverter b2c = new B2CConverter("UTF-8");
> - b2c.convert(bc, cc, true);
> + public String read(int len) throws IOException {
> + ByteChunk bc = new ByteChunk(125);
> + CharChunk cc = new CharChunk(125);
> + int read = 0;
> + while (read < len) {
> + read = read + is.read(bc.getBytes(), read, len - read);
> + }
> +
> + bc.setEnd(len);
> + b2c.convert(bc, cc, true);
> + return cc.toString();
> + }
>
> - return cc.toString();
> + public String readLine() throws IOException {
> + ByteChunk bc = new ByteChunk(125);
> + CharChunk cc = new CharChunk(125);
> + char c;
> + int i = is.read();
> + int read = 0;
> + while (i != -1) {
> + if (i != '\r' && i != '\n') {
> + bc.append((byte)i);
> + read++;
> + } else if (i == '\n') {
> + break;
> + } else if (i == '\r') {
> + if (markSupported) {
> + is.mark(2);
> + }
> + i = read();
> + if (i == -1) {
> + break;
> + } else {
> + if (i == '\n') {
> + break;
> + } else {
> + if (markSupported) {
> + is.reset();
> + }
> + }
> + }
> + }
> + i = is.read();
> + }
> + bc.setEnd(read);
> + b2c.convert(bc, cc, true);
> + return cc.toString();
> + }
> }
> }
> }
>
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
> For additional commands, e-mail: dev-help@tomcat.apache.org
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org