You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@tomcat.apache.org by ma...@apache.org on 2015/03/10 23:35:20 UTC
svn commit: r1665736 - in /tomcat/trunk/java/org/apache/tomcat/util/net:
AprEndpoint.java Nio2Endpoint.java NioEndpoint.java SocketWrapperBase.java
Author: markt
Date: Tue Mar 10 22:35:19 2015
New Revision: 1665736
URL: http://svn.apache.org/r1665736
Log:
Stop re-using the SocketWrapper
With the introduction of upgrade and non-blocking, I/O can occur on non-container threads. This makes it near impossible to track whether a SocketWrapper (== connection) is still referenced or not, making re-use a risky proposition.
Modified:
tomcat/trunk/java/org/apache/tomcat/util/net/AprEndpoint.java
tomcat/trunk/java/org/apache/tomcat/util/net/Nio2Endpoint.java
tomcat/trunk/java/org/apache/tomcat/util/net/NioEndpoint.java
tomcat/trunk/java/org/apache/tomcat/util/net/SocketWrapperBase.java
Modified: tomcat/trunk/java/org/apache/tomcat/util/net/AprEndpoint.java
URL: http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/tomcat/util/net/AprEndpoint.java?rev=1665736&r1=1665735&r2=1665736&view=diff
==============================================================================
--- tomcat/trunk/java/org/apache/tomcat/util/net/AprEndpoint.java (original)
+++ tomcat/trunk/java/org/apache/tomcat/util/net/AprEndpoint.java Tue Mar 10 22:35:19 2015
@@ -2332,7 +2332,6 @@ public class AprEndpoint extends Abstrac
if (state == Handler.SocketState.CLOSED) {
// Close socket and pool
closeSocket(socket.getSocket().longValue());
- socket.reset(null, 1);
} else if (state == Handler.SocketState.LONG) {
if (socket.isAsync()) {
waitingRequests.add(socket);
@@ -2375,12 +2374,6 @@ public class AprEndpoint extends Abstrac
}
- @Override
- protected void resetSocketBufferHandler(Long socket) {
- socketBufferHandler.reset();
- }
-
-
@Override
public int read(boolean block, byte[] b, int off, int len)
throws IOException {
Modified: tomcat/trunk/java/org/apache/tomcat/util/net/Nio2Endpoint.java
URL: http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/tomcat/util/net/Nio2Endpoint.java?rev=1665736&r1=1665735&r2=1665736&view=diff
==============================================================================
--- tomcat/trunk/java/org/apache/tomcat/util/net/Nio2Endpoint.java (original)
+++ tomcat/trunk/java/org/apache/tomcat/util/net/Nio2Endpoint.java Tue Mar 10 22:35:19 2015
@@ -118,11 +118,6 @@ public class Nio2Endpoint extends Abstra
private SynchronizedStack<SocketProcessor> processorCache;
/**
- * Cache for socket wrapper objects
- */
- private SynchronizedStack<Nio2SocketWrapper> socketWrapperCache;
-
- /**
* Bytebuffer cache, each channel holds a set of buffers (two, except for SSL holds four)
*/
private SynchronizedStack<Nio2Channel> nioChannels;
@@ -234,7 +229,6 @@ public class Nio2Endpoint extends Abstra
protected void releaseCaches() {
if (useCaches) {
- this.socketWrapperCache.clear();
this.nioChannels.clear();
this.processorCache.clear();
}
@@ -337,8 +331,6 @@ public class Nio2Endpoint extends Abstra
if (useCaches) {
processorCache = new SynchronizedStack<>(SynchronizedStack.DEFAULT_SIZE,
socketProperties.getProcessorCache());
- socketWrapperCache = new SynchronizedStack<>(SynchronizedStack.DEFAULT_SIZE,
- socketProperties.getSocketWrapperCache());
nioChannels = new SynchronizedStack<>(SynchronizedStack.DEFAULT_SIZE,
socketProperties.getBufferPool());
}
@@ -395,7 +387,6 @@ public class Nio2Endpoint extends Abstra
}
});
if (useCaches) {
- socketWrapperCache.clear();
nioChannels.clear();
processorCache.clear();
}
@@ -506,12 +497,10 @@ public class Nio2Endpoint extends Abstra
((SecureNio2Channel) channel).setSSLEngine(engine);
}
}
- Nio2SocketWrapper socketWrapper = (useCaches) ? socketWrapperCache.pop() : null;
- if (socketWrapper == null) {
- socketWrapper = new Nio2SocketWrapper(channel, this);
- }
+ Nio2SocketWrapper socketWrapper = new Nio2SocketWrapper(channel, this);
channel.reset(socket, socketWrapper);
- socketWrapper.reset(channel, getSocketProperties().getSoTimeout());
+ socketWrapper.setReadTimeout(getSocketProperties().getSoTimeout());
+ socketWrapper.setWriteTimeout(getSocketProperties().getSoTimeout());
socketWrapper.setKeepAliveLeft(Nio2Endpoint.this.getMaxKeepAliveRequests());
socketWrapper.setSecure(isSSLEnabled());
socketWrapper.setReadTimeout(getSoTimeout());
@@ -621,7 +610,6 @@ public class Nio2Endpoint extends Abstra
}
} catch (Exception ignore) {
}
- nio2Socket.reset(null, -1);
countDownConnection();
} catch (Throwable e) {
ExceptionUtils.handleThrowable(e);
@@ -934,26 +922,6 @@ public class Nio2Endpoint extends Abstra
return false;
}
- @Override
- public void reset(Nio2Channel channel, long soTimeout) {
- if (log.isDebugEnabled()) {
- log.debug("Calling [" + this + "].reset([" + channel + "],[" + soTimeout + "])",
- new Exception());
- }
- super.reset(channel, soTimeout);
- sendfileData = null;
- }
-
-
- @Override
- protected void resetSocketBufferHandler(Nio2Channel socket) {
- if (socket == null) {
- socketBufferHandler = null;
- } else {
- socketBufferHandler = socket.getBufHandler();
- }
- }
-
public void setSendfileData(SendfileData sf) { this.sendfileData = sf; }
public SendfileData getSendfileData() { return this.sendfileData; }
@@ -1672,7 +1640,6 @@ public class Nio2Endpoint extends Abstra
closeSocket(socket);
if (useCaches && running && !paused) {
nioChannels.push(socket.getSocket());
- socketWrapperCache.push((Nio2SocketWrapper) socket);
}
} else if (state == SocketState.UPGRADING) {
socket.setKeptAlive(true);
@@ -1682,7 +1649,6 @@ public class Nio2Endpoint extends Abstra
closeSocket(socket);
if (useCaches && running && !paused) {
nioChannels.push(socket.getSocket());
- socketWrapperCache.push(((Nio2SocketWrapper) socket));
}
}
} catch (OutOfMemoryError oom) {
Modified: tomcat/trunk/java/org/apache/tomcat/util/net/NioEndpoint.java
URL: http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/tomcat/util/net/NioEndpoint.java?rev=1665736&r1=1665735&r2=1665736&view=diff
==============================================================================
--- tomcat/trunk/java/org/apache/tomcat/util/net/NioEndpoint.java (original)
+++ tomcat/trunk/java/org/apache/tomcat/util/net/NioEndpoint.java Tue Mar 10 22:35:19 2015
@@ -125,11 +125,6 @@ public class NioEndpoint extends Abstrac
private SynchronizedStack<SocketProcessor> processorCache;
/**
- * Cache for key attachment objects
- */
- private SynchronizedStack<NioSocketWrapper> keyCache;
-
- /**
* Cache for poller events
*/
private SynchronizedStack<PollerEvent> eventCache;
@@ -290,7 +285,6 @@ public class NioEndpoint extends Abstrac
}
protected void releaseCaches() {
- this.keyCache.clear();
this.nioChannels.clear();
this.processorCache.clear();
if ( handler != null ) handler.recycle();
@@ -393,8 +387,6 @@ public class NioEndpoint extends Abstrac
processorCache = new SynchronizedStack<>(SynchronizedStack.DEFAULT_SIZE,
socketProperties.getProcessorCache());
- keyCache = new SynchronizedStack<>(SynchronizedStack.DEFAULT_SIZE,
- socketProperties.getKeyCache());
eventCache = new SynchronizedStack<>(SynchronizedStack.DEFAULT_SIZE,
socketProperties.getEventCache());
nioChannels = new SynchronizedStack<>(SynchronizedStack.DEFAULT_SIZE,
@@ -445,7 +437,6 @@ public class NioEndpoint extends Abstrac
}
shutdownExecutor();
eventCache.clear();
- keyCache.clear();
nioChannels.clear();
processorCache.clear();
}
@@ -900,9 +891,9 @@ public class NioEndpoint extends Abstrac
*/
public void register(final NioChannel socket) {
socket.setPoller(this);
- NioSocketWrapper key = keyCache.pop();
- final NioSocketWrapper ka = key!=null?key:new NioSocketWrapper(socket, NioEndpoint.this);
- ka.reset(this,socket,getSocketProperties().getSoTimeout());
+ NioSocketWrapper ka = new NioSocketWrapper(socket, NioEndpoint.this);
+ ka.setReadTimeout(getSocketProperties().getSoTimeout());
+ ka.setWriteTimeout(getSocketProperties().getSoTimeout());
ka.setKeepAliveLeft(NioEndpoint.this.getMaxKeepAliveRequests());
ka.setSecure(isSSLEnabled());
ka.setReadTimeout(getSoTimeout());
@@ -950,8 +941,7 @@ public class NioEndpoint extends Abstrac
}
} catch (Exception ignore) {
}
- if (ka!=null) {
- ka.reset();
+ if (ka != null) {
countDownConnection();
}
} catch (Throwable e) {
@@ -1307,48 +1297,6 @@ public class NioEndpoint extends Abstrac
pool = endpoint.getSelectorPool();
}
- public void reset(Poller poller, NioChannel channel, long soTimeout) {
- super.reset(channel, soTimeout);
-
- interestOps = 0;
- this.poller = poller;
- sendfileData = null;
- if (readLatch != null) {
- try {
- for (int i = 0; i < (int) readLatch.getCount(); i++) {
- readLatch.countDown();
- }
- } catch (Exception ignore) {
- }
- }
- readLatch = null;
- sendfileData = null;
- if (writeLatch != null) {
- try {
- for (int i = 0; i < (int) writeLatch.getCount(); i++) {
- writeLatch.countDown();
- }
- } catch (Exception ignore) {
- }
- }
- writeLatch = null;
- }
-
-
- @Override
- protected void resetSocketBufferHandler(NioChannel socket) {
- if (socket == null) {
- socketBufferHandler = null;
- } else {
- socketBufferHandler = socket.getBufHandler();
- }
- }
-
-
- public void reset() {
- reset(null,null,-1);
- }
-
public Poller getPoller() { return poller;}
public void setPoller(Poller poller){this.poller = poller;}
public int interestOps() { return interestOps;}
@@ -1731,9 +1679,6 @@ public class NioEndpoint extends Abstrac
nioChannels.push(socket);
}
socket = null;
- if (running && !paused) {
- keyCache.push(ka);
- }
}
ka = null;
} catch (Exception x) {
@@ -1748,9 +1693,6 @@ public class NioEndpoint extends Abstrac
nioChannels.push(socket);
}
socket = null;
- if (running && !paused) {
- keyCache.push(ka);
- }
ka = null;
} else {
ka.getPoller().add(socket,handshake);
Modified: tomcat/trunk/java/org/apache/tomcat/util/net/SocketWrapperBase.java
URL: http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/tomcat/util/net/SocketWrapperBase.java?rev=1665736&r1=1665735&r2=1665736&view=diff
==============================================================================
--- tomcat/trunk/java/org/apache/tomcat/util/net/SocketWrapperBase.java (original)
+++ tomcat/trunk/java/org/apache/tomcat/util/net/SocketWrapperBase.java Tue Mar 10 22:35:19 2015
@@ -326,30 +326,6 @@ public abstract class SocketWrapperBase<
}
}
- public void reset(E socket, long soTimeout) {
- async = false;
- blockingStatus = true;
- dispatches.clear();
- error = null;
- keepAliveLeft = 100;
- lastRead = System.currentTimeMillis();
- lastWrite = lastRead;
- lastAsyncStart = 0;
- asyncTimeout = -1;
- localAddr = null;
- localName = null;
- localPort = -1;
- remoteAddr = null;
- remoteHost = null;
- remotePort = -1;
- this.socket = socket;
- this.readTimeout = soTimeout;
- this.writeTimeout = soTimeout;
- upgraded = false;
- resetSocketBufferHandler(socket);
- }
-
- protected abstract void resetSocketBufferHandler(E socket);
/**
* Overridden for debug purposes. No guarantees are made about the format of
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: svn commit: r1665736 - in /tomcat/trunk/java/org/apache/tomcat/util/net:
AprEndpoint.java Nio2Endpoint.java NioEndpoint.java SocketWrapperBase.java
Posted by Mark Thomas <ma...@apache.org>.
On 11/03/2015 08:19, Mark Thomas wrote:
> On 11/03/2015 08:01, Keiichi Fujino wrote:
>> 2015-03-11 16:50 GMT+09:00 Mark Thomas <ma...@apache.org>:
>>
>>> On 11/03/2015 06:19, Keiichi Fujino wrote:
>>>> 2015-03-11 7:35 GMT+09:00 <ma...@apache.org>:
>>>>
>>>>> Author: markt
>>>>> Date: Tue Mar 10 22:35:19 2015
>>>>> New Revision: 1665736
>>>>>
>>>>> URL: http://svn.apache.org/r1665736
>>>>> Log:
>>>>> Stop re-using the SocketWrapper
>>>>> With the introduction of upgrade and non-blocking, I/O can occur on
>>>>> non-container threads. This makes it near impossible to track whether a
>>>>> SocketWrapper (== connection) is still referenced or not, making re-use
>>> a
>>>>> risky proposition.
>>>
>>> <snip/>
>>>
>>>> I think this fix causes NPE.
>>>
>>> Quite possibly.
>>>
>>>> Do we need initialization of poller and socketBufferHandler in
>>>> SocketWrapper?
>>>
>>> Again, quite possibly.
>>>
>>> I have been focussed on the crashing test case and this change didn't
>>> break that (neither did it fix it). I haven't even done a basic smoke
>>> test of these changes yet. What is the simplest way to reproduce this?
>>>
>>>
>> I think this is reproduced by simple HTTP access.
>> e.g. access to http://localhost:8080 by browser.
>
> Whoops :)
>
> Fixed.
I may have spoken too soon. Browser access works but I'm seeing lots of
errors in the logs. They might be related to my local APR changes but
maybe not. I'll take a closer look.
Mark
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: svn commit: r1665736 - in /tomcat/trunk/java/org/apache/tomcat/util/net:
AprEndpoint.java Nio2Endpoint.java NioEndpoint.java SocketWrapperBase.java
Posted by Mark Thomas <ma...@apache.org>.
On 11/03/2015 08:01, Keiichi Fujino wrote:
> 2015-03-11 16:50 GMT+09:00 Mark Thomas <ma...@apache.org>:
>
>> On 11/03/2015 06:19, Keiichi Fujino wrote:
>>> 2015-03-11 7:35 GMT+09:00 <ma...@apache.org>:
>>>
>>>> Author: markt
>>>> Date: Tue Mar 10 22:35:19 2015
>>>> New Revision: 1665736
>>>>
>>>> URL: http://svn.apache.org/r1665736
>>>> Log:
>>>> Stop re-using the SocketWrapper
>>>> With the introduction of upgrade and non-blocking, I/O can occur on
>>>> non-container threads. This makes it near impossible to track whether a
>>>> SocketWrapper (== connection) is still referenced or not, making re-use
>> a
>>>> risky proposition.
>>
>> <snip/>
>>
>>> I think this fix causes NPE.
>>
>> Quite possibly.
>>
>>> Do we need initialization of poller and socketBufferHandler in
>>> SocketWrapper?
>>
>> Again, quite possibly.
>>
>> I have been focussed on the crashing test case and this change didn't
>> break that (neither did it fix it). I haven't even done a basic smoke
>> test of these changes yet. What is the simplest way to reproduce this?
>>
>>
> I think this is reproduced by simple HTTP access.
> e.g. access to http://localhost:8080 by browser.
Whoops :)
Fixed.
Mark
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: svn commit: r1665736 - in /tomcat/trunk/java/org/apache/tomcat/util/net:
AprEndpoint.java Nio2Endpoint.java NioEndpoint.java SocketWrapperBase.java
Posted by Keiichi Fujino <kf...@apache.org>.
2015-03-11 16:50 GMT+09:00 Mark Thomas <ma...@apache.org>:
> On 11/03/2015 06:19, Keiichi Fujino wrote:
> > 2015-03-11 7:35 GMT+09:00 <ma...@apache.org>:
> >
> >> Author: markt
> >> Date: Tue Mar 10 22:35:19 2015
> >> New Revision: 1665736
> >>
> >> URL: http://svn.apache.org/r1665736
> >> Log:
> >> Stop re-using the SocketWrapper
> >> With the introduction of upgrade and non-blocking, I/O can occur on
> >> non-container threads. This makes it near impossible to track whether a
> >> SocketWrapper (== connection) is still referenced or not, making re-use
> a
> >> risky proposition.
>
> <snip/>
>
> > I think this fix causes NPE.
>
> Quite possibly.
>
> > Do we need initialization of poller and socketBufferHandler in
> > SocketWrapper?
>
> Again, quite possibly.
>
> I have been focussed on the crashing test case and this change didn't
> break that (neither did it fix it). I haven't even done a basic smoke
> test of these changes yet. What is the simplest way to reproduce this?
>
>
I think this is reproduced by simple HTTP access.
e.g. access to http://localhost:8080 by browser.
> Mark
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
> For additional commands, e-mail: dev-help@tomcat.apache.org
>
> --
> Keiichi.Fujino
> <de...@tomcat.apache.org>
> <de...@tomcat.apache.org>
>
Re: svn commit: r1665736 - in /tomcat/trunk/java/org/apache/tomcat/util/net:
AprEndpoint.java Nio2Endpoint.java NioEndpoint.java SocketWrapperBase.java
Posted by Mark Thomas <ma...@apache.org>.
On 11/03/2015 06:19, Keiichi Fujino wrote:
> 2015-03-11 7:35 GMT+09:00 <ma...@apache.org>:
>
>> Author: markt
>> Date: Tue Mar 10 22:35:19 2015
>> New Revision: 1665736
>>
>> URL: http://svn.apache.org/r1665736
>> Log:
>> Stop re-using the SocketWrapper
>> With the introduction of upgrade and non-blocking, I/O can occur on
>> non-container threads. This makes it near impossible to track whether a
>> SocketWrapper (== connection) is still referenced or not, making re-use a
>> risky proposition.
<snip/>
> I think this fix causes NPE.
Quite possibly.
> Do we need initialization of poller and socketBufferHandler in
> SocketWrapper?
Again, quite possibly.
I have been focussed on the crashing test case and this change didn't
break that (neither did it fix it). I haven't even done a basic smoke
test of these changes yet. What is the simplest way to reproduce this?
Mark
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: svn commit: r1665736 - in /tomcat/trunk/java/org/apache/tomcat/util/net:
AprEndpoint.java Nio2Endpoint.java NioEndpoint.java SocketWrapperBase.java
Posted by Keiichi Fujino <kf...@apache.org>.
2015-03-11 7:35 GMT+09:00 <ma...@apache.org>:
> Author: markt
> Date: Tue Mar 10 22:35:19 2015
> New Revision: 1665736
>
> URL: http://svn.apache.org/r1665736
> Log:
> Stop re-using the SocketWrapper
> With the introduction of upgrade and non-blocking, I/O can occur on
> non-container threads. This makes it near impossible to track whether a
> SocketWrapper (== connection) is still referenced or not, making re-use a
> risky proposition.
>
> Modified:
> tomcat/trunk/java/org/apache/tomcat/util/net/AprEndpoint.java
> tomcat/trunk/java/org/apache/tomcat/util/net/Nio2Endpoint.java
> tomcat/trunk/java/org/apache/tomcat/util/net/NioEndpoint.java
> tomcat/trunk/java/org/apache/tomcat/util/net/SocketWrapperBase.java
>
> Modified: tomcat/trunk/java/org/apache/tomcat/util/net/AprEndpoint.java
> URL:
> http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/tomcat/util/net/AprEndpoint.java?rev=1665736&r1=1665735&r2=1665736&view=diff
>
> ==============================================================================
> --- tomcat/trunk/java/org/apache/tomcat/util/net/AprEndpoint.java
> (original)
> +++ tomcat/trunk/java/org/apache/tomcat/util/net/AprEndpoint.java Tue Mar
> 10 22:35:19 2015
> @@ -2332,7 +2332,6 @@ public class AprEndpoint extends Abstrac
> if (state == Handler.SocketState.CLOSED) {
> // Close socket and pool
> closeSocket(socket.getSocket().longValue());
> - socket.reset(null, 1);
> } else if (state == Handler.SocketState.LONG) {
> if (socket.isAsync()) {
> waitingRequests.add(socket);
> @@ -2375,12 +2374,6 @@ public class AprEndpoint extends Abstrac
> }
>
>
> - @Override
> - protected void resetSocketBufferHandler(Long socket) {
> - socketBufferHandler.reset();
> - }
> -
> -
> @Override
> public int read(boolean block, byte[] b, int off, int len)
> throws IOException {
>
> Modified: tomcat/trunk/java/org/apache/tomcat/util/net/Nio2Endpoint.java
> URL:
> http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/tomcat/util/net/Nio2Endpoint.java?rev=1665736&r1=1665735&r2=1665736&view=diff
>
> ==============================================================================
> --- tomcat/trunk/java/org/apache/tomcat/util/net/Nio2Endpoint.java
> (original)
> +++ tomcat/trunk/java/org/apache/tomcat/util/net/Nio2Endpoint.java Tue Mar
> 10 22:35:19 2015
> @@ -118,11 +118,6 @@ public class Nio2Endpoint extends Abstra
> private SynchronizedStack<SocketProcessor> processorCache;
>
> /**
> - * Cache for socket wrapper objects
> - */
> - private SynchronizedStack<Nio2SocketWrapper> socketWrapperCache;
> -
> - /**
> * Bytebuffer cache, each channel holds a set of buffers (two, except
> for SSL holds four)
> */
> private SynchronizedStack<Nio2Channel> nioChannels;
> @@ -234,7 +229,6 @@ public class Nio2Endpoint extends Abstra
>
> protected void releaseCaches() {
> if (useCaches) {
> - this.socketWrapperCache.clear();
> this.nioChannels.clear();
> this.processorCache.clear();
> }
> @@ -337,8 +331,6 @@ public class Nio2Endpoint extends Abstra
> if (useCaches) {
> processorCache = new
> SynchronizedStack<>(SynchronizedStack.DEFAULT_SIZE,
> socketProperties.getProcessorCache());
> - socketWrapperCache = new
> SynchronizedStack<>(SynchronizedStack.DEFAULT_SIZE,
> - socketProperties.getSocketWrapperCache());
> nioChannels = new
> SynchronizedStack<>(SynchronizedStack.DEFAULT_SIZE,
> socketProperties.getBufferPool());
> }
> @@ -395,7 +387,6 @@ public class Nio2Endpoint extends Abstra
> }
> });
> if (useCaches) {
> - socketWrapperCache.clear();
> nioChannels.clear();
> processorCache.clear();
> }
> @@ -506,12 +497,10 @@ public class Nio2Endpoint extends Abstra
> ((SecureNio2Channel) channel).setSSLEngine(engine);
> }
> }
> - Nio2SocketWrapper socketWrapper = (useCaches) ?
> socketWrapperCache.pop() : null;
> - if (socketWrapper == null) {
> - socketWrapper = new Nio2SocketWrapper(channel, this);
> - }
> + Nio2SocketWrapper socketWrapper = new
> Nio2SocketWrapper(channel, this);
> channel.reset(socket, socketWrapper);
> - socketWrapper.reset(channel,
> getSocketProperties().getSoTimeout());
> +
> socketWrapper.setReadTimeout(getSocketProperties().getSoTimeout());
> +
> socketWrapper.setWriteTimeout(getSocketProperties().getSoTimeout());
>
> socketWrapper.setKeepAliveLeft(Nio2Endpoint.this.getMaxKeepAliveRequests());
> socketWrapper.setSecure(isSSLEnabled());
> socketWrapper.setReadTimeout(getSoTimeout());
> @@ -621,7 +610,6 @@ public class Nio2Endpoint extends Abstra
> }
> } catch (Exception ignore) {
> }
> - nio2Socket.reset(null, -1);
> countDownConnection();
> } catch (Throwable e) {
> ExceptionUtils.handleThrowable(e);
> @@ -934,26 +922,6 @@ public class Nio2Endpoint extends Abstra
> return false;
> }
>
> - @Override
> - public void reset(Nio2Channel channel, long soTimeout) {
> - if (log.isDebugEnabled()) {
> - log.debug("Calling [" + this + "].reset([" + channel +
> "],[" + soTimeout + "])",
> - new Exception());
> - }
> - super.reset(channel, soTimeout);
> - sendfileData = null;
> - }
> -
> -
> - @Override
> - protected void resetSocketBufferHandler(Nio2Channel socket) {
> - if (socket == null) {
> - socketBufferHandler = null;
> - } else {
> - socketBufferHandler = socket.getBufHandler();
> - }
> - }
> -
>
> public void setSendfileData(SendfileData sf) { this.sendfileData
> = sf; }
> public SendfileData getSendfileData() { return this.sendfileData;
> }
> @@ -1672,7 +1640,6 @@ public class Nio2Endpoint extends Abstra
> closeSocket(socket);
> if (useCaches && running && !paused) {
> nioChannels.push(socket.getSocket());
> -
> socketWrapperCache.push((Nio2SocketWrapper) socket);
> }
> } else if (state == SocketState.UPGRADING) {
> socket.setKeptAlive(true);
> @@ -1682,7 +1649,6 @@ public class Nio2Endpoint extends Abstra
> closeSocket(socket);
> if (useCaches && running && !paused) {
> nioChannels.push(socket.getSocket());
> - socketWrapperCache.push(((Nio2SocketWrapper)
> socket));
> }
> }
> } catch (OutOfMemoryError oom) {
>
> Modified: tomcat/trunk/java/org/apache/tomcat/util/net/NioEndpoint.java
> URL:
> http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/tomcat/util/net/NioEndpoint.java?rev=1665736&r1=1665735&r2=1665736&view=diff
>
> ==============================================================================
> --- tomcat/trunk/java/org/apache/tomcat/util/net/NioEndpoint.java
> (original)
> +++ tomcat/trunk/java/org/apache/tomcat/util/net/NioEndpoint.java Tue Mar
> 10 22:35:19 2015
> @@ -125,11 +125,6 @@ public class NioEndpoint extends Abstrac
> private SynchronizedStack<SocketProcessor> processorCache;
>
> /**
> - * Cache for key attachment objects
> - */
> - private SynchronizedStack<NioSocketWrapper> keyCache;
> -
> - /**
> * Cache for poller events
> */
> private SynchronizedStack<PollerEvent> eventCache;
> @@ -290,7 +285,6 @@ public class NioEndpoint extends Abstrac
> }
>
> protected void releaseCaches() {
> - this.keyCache.clear();
> this.nioChannels.clear();
> this.processorCache.clear();
> if ( handler != null ) handler.recycle();
> @@ -393,8 +387,6 @@ public class NioEndpoint extends Abstrac
>
> processorCache = new
> SynchronizedStack<>(SynchronizedStack.DEFAULT_SIZE,
> socketProperties.getProcessorCache());
> - keyCache = new
> SynchronizedStack<>(SynchronizedStack.DEFAULT_SIZE,
> - socketProperties.getKeyCache());
> eventCache = new
> SynchronizedStack<>(SynchronizedStack.DEFAULT_SIZE,
> socketProperties.getEventCache());
> nioChannels = new
> SynchronizedStack<>(SynchronizedStack.DEFAULT_SIZE,
> @@ -445,7 +437,6 @@ public class NioEndpoint extends Abstrac
> }
> shutdownExecutor();
> eventCache.clear();
> - keyCache.clear();
> nioChannels.clear();
> processorCache.clear();
> }
> @@ -900,9 +891,9 @@ public class NioEndpoint extends Abstrac
> */
> public void register(final NioChannel socket) {
> socket.setPoller(this);
> - NioSocketWrapper key = keyCache.pop();
> - final NioSocketWrapper ka = key!=null?key:new
> NioSocketWrapper(socket, NioEndpoint.this);
> - ka.reset(this,socket,getSocketProperties().getSoTimeout());
> + NioSocketWrapper ka = new NioSocketWrapper(socket,
> NioEndpoint.this);
> + ka.setReadTimeout(getSocketProperties().getSoTimeout());
> + ka.setWriteTimeout(getSocketProperties().getSoTimeout());
>
> ka.setKeepAliveLeft(NioEndpoint.this.getMaxKeepAliveRequests());
> ka.setSecure(isSSLEnabled());
> ka.setReadTimeout(getSoTimeout());
> @@ -950,8 +941,7 @@ public class NioEndpoint extends Abstrac
> }
> } catch (Exception ignore) {
> }
> - if (ka!=null) {
> - ka.reset();
> + if (ka != null) {
> countDownConnection();
> }
> } catch (Throwable e) {
> @@ -1307,48 +1297,6 @@ public class NioEndpoint extends Abstrac
> pool = endpoint.getSelectorPool();
> }
>
> - public void reset(Poller poller, NioChannel channel, long
> soTimeout) {
> - super.reset(channel, soTimeout);
> -
> - interestOps = 0;
> - this.poller = poller;
> - sendfileData = null;
> - if (readLatch != null) {
> - try {
> - for (int i = 0; i < (int) readLatch.getCount(); i++) {
> - readLatch.countDown();
> - }
> - } catch (Exception ignore) {
> - }
> - }
> - readLatch = null;
> - sendfileData = null;
> - if (writeLatch != null) {
> - try {
> - for (int i = 0; i < (int) writeLatch.getCount(); i++)
> {
> - writeLatch.countDown();
> - }
> - } catch (Exception ignore) {
> - }
> - }
> - writeLatch = null;
> - }
> -
> -
> - @Override
> - protected void resetSocketBufferHandler(NioChannel socket) {
> - if (socket == null) {
> - socketBufferHandler = null;
> - } else {
> - socketBufferHandler = socket.getBufHandler();
> - }
> - }
> -
> -
> - public void reset() {
> - reset(null,null,-1);
> - }
> -
> public Poller getPoller() { return poller;}
> public void setPoller(Poller poller){this.poller = poller;}
> public int interestOps() { return interestOps;}
> @@ -1731,9 +1679,6 @@ public class NioEndpoint extends Abstrac
> nioChannels.push(socket);
> }
> socket = null;
> - if (running && !paused) {
> - keyCache.push(ka);
> - }
> }
> ka = null;
> } catch (Exception x) {
> @@ -1748,9 +1693,6 @@ public class NioEndpoint extends Abstrac
> nioChannels.push(socket);
> }
> socket = null;
> - if (running && !paused) {
> - keyCache.push(ka);
> - }
> ka = null;
> } else {
> ka.getPoller().add(socket,handshake);
>
> Modified:
> tomcat/trunk/java/org/apache/tomcat/util/net/SocketWrapperBase.java
> URL:
> http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/tomcat/util/net/SocketWrapperBase.java?rev=1665736&r1=1665735&r2=1665736&view=diff
>
> ==============================================================================
> --- tomcat/trunk/java/org/apache/tomcat/util/net/SocketWrapperBase.java
> (original)
> +++ tomcat/trunk/java/org/apache/tomcat/util/net/SocketWrapperBase.java
> Tue Mar 10 22:35:19 2015
> @@ -326,30 +326,6 @@ public abstract class SocketWrapperBase<
> }
> }
>
> - public void reset(E socket, long soTimeout) {
> - async = false;
> - blockingStatus = true;
> - dispatches.clear();
> - error = null;
> - keepAliveLeft = 100;
> - lastRead = System.currentTimeMillis();
> - lastWrite = lastRead;
> - lastAsyncStart = 0;
> - asyncTimeout = -1;
> - localAddr = null;
> - localName = null;
> - localPort = -1;
> - remoteAddr = null;
> - remoteHost = null;
> - remotePort = -1;
> - this.socket = socket;
> - this.readTimeout = soTimeout;
> - this.writeTimeout = soTimeout;
> - upgraded = false;
> - resetSocketBufferHandler(socket);
> - }
> -
> - protected abstract void resetSocketBufferHandler(E socket);
>
> /**
> * Overridden for debug purposes. No guarantees are made about the
> format of
>
>
I think this fix causes NPE.
Do we need initialization of poller and socketBufferHandler in
SocketWrapper?
> --
> Keiichi.Fujino
> <de...@tomcat.apache.org>
Re: svn commit: r1665736 - in /tomcat/trunk/java/org/apache/tomcat/util/net:
AprEndpoint.java Nio2Endpoint.java NioEndpoint.java SocketWrapperBase.java
Posted by Mark Thomas <ma...@apache.org>.
On 11/03/2015 08:07, Rémy Maucherat wrote:
> 2015-03-10 23:35 GMT+01:00 <ma...@apache.org>:
>
>> Author: markt
>> Date: Tue Mar 10 22:35:19 2015
>> New Revision: 1665736
>>
>> URL: http://svn.apache.org/r1665736
>> Log:
>> Stop re-using the SocketWrapper
>> With the introduction of upgrade and non-blocking, I/O can occur on
>> non-container threads. This makes it near impossible to track whether a
>> SocketWrapper (== connection) is still referenced or not, making re-use a
>> risky proposition.
>>
>> Modified:
>> tomcat/trunk/java/org/apache/tomcat/util/net/AprEndpoint.java
>> tomcat/trunk/java/org/apache/tomcat/util/net/Nio2Endpoint.java
>> tomcat/trunk/java/org/apache/tomcat/util/net/NioEndpoint.java
>> tomcat/trunk/java/org/apache/tomcat/util/net/SocketWrapperBase.java
>>
>
> I had tested enabling reuse of the wrapper with NIO2, and it didn't change
> the ab results. Same for using the useCaches flag, so there's probably no
> point either reusing the other objects in endpoint.
I while back I did some GC testing under load and I think some of those
caches were introduced to reduce the GC churn.
Given none of them are obviously causing problems my instinct is to
leave them as is for now.
My current TODO list is:
- fix this APR crash
- fix the remaining bugs that impact 8.0.x
- do an 8.0.x release
- do some performance testing
I can add looking at the performance and GC benefits (or not) of these
caches to that last item although I'd be more than happy to if someone
beat me to it.
Mark
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: svn commit: r1665736 - in /tomcat/trunk/java/org/apache/tomcat/util/net:
AprEndpoint.java Nio2Endpoint.java NioEndpoint.java SocketWrapperBase.java
Posted by Rémy Maucherat <re...@apache.org>.
2015-03-10 23:35 GMT+01:00 <ma...@apache.org>:
> Author: markt
> Date: Tue Mar 10 22:35:19 2015
> New Revision: 1665736
>
> URL: http://svn.apache.org/r1665736
> Log:
> Stop re-using the SocketWrapper
> With the introduction of upgrade and non-blocking, I/O can occur on
> non-container threads. This makes it near impossible to track whether a
> SocketWrapper (== connection) is still referenced or not, making re-use a
> risky proposition.
>
> Modified:
> tomcat/trunk/java/org/apache/tomcat/util/net/AprEndpoint.java
> tomcat/trunk/java/org/apache/tomcat/util/net/Nio2Endpoint.java
> tomcat/trunk/java/org/apache/tomcat/util/net/NioEndpoint.java
> tomcat/trunk/java/org/apache/tomcat/util/net/SocketWrapperBase.java
>
I had tested enabling reuse of the wrapper with NIO2, and it didn't change
the ab results. Same for using the useCaches flag, so there's probably no
point either reusing the other objects in endpoint.
Rémy