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
_______________________________________________
jboss-user mailing list
jboss-user@lists.jboss.org
https://lists.jboss.org/mailman/listinfo/jboss-user

Reply via email to