What is the NMS issue that this fix is for? You realize that locking around an atomic compare and set operation is pretty pointless right? Wouldn't it be better if close just returned if its already closed instead of propagating and exception that might not be expected out to an exception handler at times like client shutdown?
RegardsOn Tue, 2010-12-21 at 21:59 +0000, [email protected] wrote: > Author: jgomes > Date: Tue Dec 21 21:59:54 2010 > New Revision: 1051673 > > URL: http://svn.apache.org/viewvc?rev=1051673&view=rev > Log: > Call the exceptionHandler in Oneway() instead of throwing an exception if the > connection is closed. This is more consistent behavior. > Protect access to the closed variable in Close() using the myLock critical > section lock. Even though closed is an Atomic boolean, that code block > should be protected by the critical section lock for consistency. > All passing unit tests continue to pass with these changes. > > Modified: > > activemq/activemq-dotnet/Apache.NMS.ActiveMQ/trunk/src/main/csharp/Transport/Tcp/TcpTransport.cs > > Modified: > activemq/activemq-dotnet/Apache.NMS.ActiveMQ/trunk/src/main/csharp/Transport/Tcp/TcpTransport.cs > URL: > http://svn.apache.org/viewvc/activemq/activemq-dotnet/Apache.NMS.ActiveMQ/trunk/src/main/csharp/Transport/Tcp/TcpTransport.cs?rev=1051673&r1=1051672&r2=1051673&view=diff > ============================================================================== > --- > activemq/activemq-dotnet/Apache.NMS.ActiveMQ/trunk/src/main/csharp/Transport/Tcp/TcpTransport.cs > (original) > +++ > activemq/activemq-dotnet/Apache.NMS.ActiveMQ/trunk/src/main/csharp/Transport/Tcp/TcpTransport.cs > Tue Dec 21 21:59:54 2010 > @@ -54,15 +54,15 @@ namespace Apache.NMS.ActiveMQ.Transport. > this.wireformat = wireformat; > } > > - ~TcpTransport() > + ~TcpTransport() > { > Dispose(false); > } > - > - protected virtual Stream CreateSocketStream() > - { > - return new NetworkStream(socket); > - } > + > + protected virtual Stream CreateSocketStream() > + { > + return new NetworkStream(socket); > + } > > /// <summary> > /// Method Start > @@ -87,15 +87,15 @@ namespace Apache.NMS.ActiveMQ.Transport. > > started = true; > > - // Initialize our Read and Writer instances. Its not > actually necessary > - // to have two distinct NetworkStream instances but for > now the TcpTransport > - // will continue to do so for legacy reasons. > - socketWriter = new > EndianBinaryWriter(CreateSocketStream()); > - socketReader = new > EndianBinaryReader(CreateSocketStream()); > + // Initialize our Read and Writer > instances. Its not actually necessary > + // to have two distinct NetworkStream > instances but for now the TcpTransport > + // will continue to do so for legacy > reasons. > + socketWriter = new > EndianBinaryWriter(CreateSocketStream()); > + socketReader = new > EndianBinaryReader(CreateSocketStream()); > > // now lets create the background read > thread > - readThread = new Thread(new > ThreadStart(ReadLoop)) {IsBackground = true}; > - readThread.Start(); > + readThread = new Thread(new > ThreadStart(ReadLoop)) { IsBackground = true }; > + readThread.Start(); > } > } > } > @@ -120,7 +120,8 @@ namespace Apache.NMS.ActiveMQ.Transport. > { > if(closed.Value) > { > - throw new > InvalidOperationException("Error writing to broker. Transport connection is > closed."); > + this.exceptionHandler(this, new > InvalidOperationException("Error writing to broker. Transport connection is > closed.")); > + return; > } > > if(command is ShutdownInfo) > @@ -165,9 +166,9 @@ namespace Apache.NMS.ActiveMQ.Transport. > > public void Close() > { > - if(closed.CompareAndSet(false, true)) > + lock(myLock) > { > - lock(myLock) > + if(closed.CompareAndSet(false, true)) > { > try > { > @@ -224,7 +225,7 @@ namespace Apache.NMS.ActiveMQ.Transport. > > readThread.Abort(); > } > } > - > + > readThread = null; > } > > @@ -356,23 +357,23 @@ namespace Apache.NMS.ActiveMQ.Transport. > > public Object Narrow(Type type) > { > - return this.GetType().Equals(type) ? this : null; > + return this.GetType().Equals(type) ? this : null; > } > > - public bool IsReconnectSupported > + public bool IsReconnectSupported > { > - get{ return false; } > + get { return false; } > } > - > - public bool IsUpdateURIsSupported > + > + public bool IsUpdateURIsSupported > { > - get{ return false; } > + get { return false; } > } > - > + > public void UpdateURIs(bool rebalance, Uri[] updatedURIs) > { > throw new IOException(); > - } > + } > } > } > > > -- Tim Bish ------------ FuseSource Email: [email protected] Web: http://fusesource.com Twitter: tabish121 Blog: http://timbish.blogspot.com/
