Re: svn commit: r1662226 - /tomcat/trunk/java/org/apache/tomcat/websocket/server/WsRemoteEndpointImplServer.java

2015-02-25 Thread Rémy Maucherat
2015-02-25 16:07 GMT+01:00 Felix Schumacher <
felix.schumac...@internetallee.de>:

> Am 25.02.2015 15:34, schrieb r...@apache.org:
> +first = false;
>  This is not really threadsafe. What happens when more than one thread
> call this method while first==false? They could (probably really really
> rarely) happen to get into this if condition at the "same" time, set first
> to false and do nothing. My reading would be, that this should happen only
> for the first call. Maybe thread safety isn't an issue, but do we need
> volatile in that case?
>
> If it actually fixes something, it'll be improved and then fully redone.

Rémy


Re: svn commit: r1662226 - /tomcat/trunk/java/org/apache/tomcat/websocket/server/WsRemoteEndpointImplServer.java

2015-02-25 Thread Felix Schumacher

Am 25.02.2015 15:34, schrieb r...@apache.org:

Author: remm
Date: Wed Feb 25 14:34:47 2015
New Revision: 1662226

URL: http://svn.apache.org/r1662226
Log:
Add a hack to skip the initial write event since it is not really
useful for websockets. Looking at the CI history and the concurrency
results for write, the problems with TestWebSocketFrameClient may
originate from 1660609.

Modified:

tomcat/trunk/java/org/apache/tomcat/websocket/server/WsRemoteEndpointImplServer.java

Modified:
tomcat/trunk/java/org/apache/tomcat/websocket/server/WsRemoteEndpointImplServer.java
URL:
http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/tomcat/websocket/server/WsRemoteEndpointImplServer.java?rev=1662226&r1=1662225&r2=1662226&view=diff
==
---
tomcat/trunk/java/org/apache/tomcat/websocket/server/WsRemoteEndpointImplServer.java
(original)
+++
tomcat/trunk/java/org/apache/tomcat/websocket/server/WsRemoteEndpointImplServer.java
Wed Feb 25 14:34:47 2015
@@ -59,7 +59,7 @@ public class WsRemoteEndpointImplServer

 private volatile long timeoutExpiry = -1;
 private volatile boolean close;
-
+private volatile boolean first = true;

 public WsRemoteEndpointImplServer(ServletInputStream sis,
ServletOutputStream sos,
 WsServerContainer serverContainer) {
@@ -87,51 +87,57 @@ public class WsRemoteEndpointImplServer


 public void onWritePossible(boolean useDispatch) {
-if (buffers == null) {
-// Servlet 3.1 will call the write listener once even if 
nothing

-// was written
-return;
-}
-boolean complete = false;
-try {
-// If this is false there will be a call back when it is 
true

-while (sos.isReady()) {
-complete = true;
-for (ByteBuffer buffer : buffers) {
-if (buffer.hasRemaining()) {
-complete = false;
-sos.write(buffer.array(), 
buffer.arrayOffset(),

-buffer.limit());
-buffer.position(buffer.limit());
-break;
+ByteBuffer[] buffers = this.buffers;
+if (first) {
+// Wait for the fist message to do something

fist => first


+first = false;
This is not really threadsafe. What happens when more than one thread 
call this method while first==false? They could (probably really really 
rarely) happen to get into this if condition at the "same" time, set 
first to false and do nothing. My reading would be, that this should 
happen only for the first call. Maybe thread safety isn't an issue, but 
do we need volatile in that case?




+} else {
+if (buffers == null) {
+// Servlet 3.1 will call the write listener once even
if nothing
+// was written
+return;
+}
+boolean complete = false;
+try {
+// If this is false there will be a call back when it 
is true

+while (sos.isReady()) {
+complete = true;
+for (ByteBuffer buffer : buffers) {
+if (buffer.hasRemaining()) {
+complete = false;
+sos.write(buffer.array(), 
buffer.arrayOffset(),

+buffer.limit());
+buffer.position(buffer.limit());
+break;
+}
The switching of the variable complete from false to true to false makes 
me a bit dizzy.
I would have put the for loop into a method and returned the 
complete-status from there.

But this is purely cosmetic.

Regards
 Felix


 }
-}
-if (complete) {
-sos.flush();
-complete = sos.isReady();
 if (complete) {
-wsWriteTimeout.unregister(this);
-clearHandler(null, useDispatch);
-if (close) {
-close();
+sos.flush();
+complete = sos.isReady();
+if (complete) {
+wsWriteTimeout.unregister(this);
+clearHandler(null, useDispatch);
+if (close) {
+close();
+}
 }
+break;
 }
-break;
 }
+} catch (IOException | IllegalStateException e) {
+wsWriteTimeout.unregister(this);
+clearHandler(e, useDispatch);
+close();
 }
-} catch (IOException | IllegalStateException e) {
-