[jboss-user] [JBoss Cache: Core Edition] - Re: TcpDelegatingCacheLoader

[lovelyliatroim]

Hi Manik,

Just seen your entry here to resolved


     [ https://jira.jboss.org/jira/browse/JBCACHE-1451?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]

Had a look at your fix here

http://fisheye.jboss.org/browse/JBossCache/core/trunk/src/main/java/org/jboss/cache/loader/TcpDelegatingCacheLoader.java?r=7378

Is that the fixed version??

If so you there is a bug.

Let me explain



  | protected Map<Object, Object> _get(Fqn name) throws Exception
  |    {
  |       synchronized (this)
  |       {
  |          out.reset();
  | 
  |          out.writeByte(TcpCacheOperations.GET);
  |          out.writeObject(name);
  |          out.flush();
  |          Object retval = in.readObject();
  |          if (retval instanceof Exception)
  |          {
  |             throw (Exception) retval;
  |          }
  |          return (Map) retval;
  |       }
  |    }
  | 
  | 

Lets take this sample here, your locking is better but you make the mistake in throwing the IOException to the calling method. Once you throw it you loose your lock and when catching the IOException you dont synch on the restart, so picture this

T1 gets lock, 
T2 waits for lock
T1 Times out or server socket is down and throws exception
T2 now has lock and times out
T2 throws exception
T1 calls restart from invokeWithRetries
T3 acquires lock and writes but doesnt read
T2 calls restart from invokeWithRetries
T3 now attempts to read response

You need to clean up in the _get() while you still have the lock

Something like this




  | protected Map<Object, Object> _get(Fqn name) throws Exception
  | 	{
  | 		synchronized (this){
  | 			try {
  | 
  | 				out.reset();
  | 
  | 				out.writeByte(TcpCacheOperations.GET);
  | 
  | 				out.writeObject(name);
  | 				out.flush();
  | 				Object retval = in.readObject();
  | 				if(log.isDebugEnabled()){
  | 					log.debug("Path=" +name +" Return Val"+name);
  | 				}
  | 
  | 				if (retval instanceof Exception)
  | 				{
  | 					throw (Exception) retval;
  | 				}
  | 				return (Map) retval;
  | 			} catch (IOException e) {
  | 				log.info("IOEXception occurred. Will attempt to re-create connection",e);
  | 				try{
  | 					restart();
  | 				}catch(IOException ex){
  | 					log.error("Failed to reconnect to remote server, it might be down??"+ex);
  | 				}
  | 			}
  | 		}
  | 
  | 		return null;
  | 	}
  | 


That way only one thread calls the restart and once reset it should be ok for the next thread.


Next thing i see is the start method



  | public void start() throws IOException
  |    {
  |       try
  |       {
  |          sock = new Socket(config.getHost(), config.getPort());
  |          sock.setSoTimeout(config.getReadTimeout());
  |          out = new ObjectOutputStream(new BufferedOutputStream(sock.getOutputStream()));
  |          out.flush();
  |          in = new ObjectInputStream(new BufferedInputStream(sock.getInputStream()));
  |       }
  |       catch (ConnectException ce)
  |       {
  |          log.info("Unable to connect to TCP socket on interface " + config.getHost() + " and port " + config.getPort());
  |          throw ce;
  |       }
  |    }
  | 


I would change this also, I know i wrote it originally but I would change it to something like this



  | 	public void start() throws IOException
  | 	{
  | 		log.info("Attempting to start a connection to server");
  | 
  | 		InetSocketAddress address = new InetSocketAddress(config.getHost(),config.getPort());
  | 		sock = new Socket();
  | 		sock.setSoTimeout(config.getReadTimeout());
  | 		sock.connect(address, config.getReadTimeout());
  | 		out = new ObjectOutputStream(new BufferedOutputStream(sock.getOutputStream()));
  | 		out.flush();
  | 		in = new ObjectInputStream(new BufferedInputStream(sock.getInputStream()));
  | 	}
  | 
  | 

Reason being I dont like this



  | sock = new Socket(config.getHost(), config.getPort());
  | 
This constructor from the API says that


anonymous wrote : 
  | Creates a stream socket and connects it to the specified port number at the specified IP address. 
  | 

Now at this stage we have no timeout set, so does that mean we dont timeout and can wait to establish a connection for no matter how long it takes??? Other way is safer, we set the timeout before we make the connection.


Just my 2 cents on it, hope it helps. Ignore me if I am looking at the wrong file but would appreciate if you posted the right link for me so I can look at the fix for it.

Regards,
LL


View the original post : http://www.jboss.com/index.html?module=bb&op=viewtopic&p=4201814#4201814

Reply to the post : http://www.jboss.com/index.html?module=bb&op=posting&mode=reply&p=4201814