activemq-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Timothy Bish <tabish...@gmail.com>
Subject Re: svn commit: r1051673 - /activemq/activemq-dotnet/Apache.NMS.ActiveMQ/trunk/src/main/csharp/Transport/Tcp/TcpTransport.cs
Date Tue, 21 Dec 2010 22:13:52 GMT
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, jgomes@apache.org 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: tim.bish@fusesource.com
Web: http://fusesource.com
Twitter: tabish121
Blog: http://timbish.blogspot.com/



Mime
View raw message