Link to home
Start Free TrialLog in
Avatar of narrav
narrav

asked on

Java - MultiThreading and exception

Hi All,

Sorry I have just started doing java, so trying to get some knowledge as well as accomplish some tasks on my project.

I have just got a new project which was developed before by some other team. But after that this project was shut-down and whole team was let go because of some financial issues. But now my company to re-open this project, and want to hire people back, but before they do that, they have asked me to accomplish some tasks on this project..

One of them is - to make this java project multi-thread. This project was based on singleton pattern and most of variables were declared as instance. Due to which this application didn't support multi-thread model.

so I was changing this kind of variables to local method variables.. to make it multi-thread.

synchronizing the methods is also a solution but I cannot do it because other task that I have been assigned to make this project live again is performance. If I synchronize the methods, I will lose performance.

I have changed most of variables as local, but I am getting below exception from one of the private method:

java.util.ConcurrentModificationException
      at java.lang.Throwable.fillInStackTrace(Native Method)
      at java.lang.Throwable.fillInStackTrace(Compiled Code)
      at java.lang.Throwable.<init>(Compiled Code)
      at java.lang.Exception.<init>(Compiled Code)
      at java.lang.RuntimeException.<init>(Compiled Code)
      at java.util.ConcurrentModificationException.<init>(ConcurrentModificationException.java:48)
      at java.util.AbstractList$Itr.remove(Compiled Code)
      at com.mapper.sybase.ChargeProfileMapperImpl.filterIneligibleCharges(Compiled Code)
      at com.mapper.sybase.ChargeProfileMapperImpl.find(Compiled Code)
      at com.ClientProfileService.getProfiledCharges(Compiled Code)
      at com.ccs.ChargeCalculationService.doCalculation(Compiled Code)
      at com.test.TestDriver.testCalculation(Compiled Code)
      at com.test.ThreadCpacsCore.run(ThreadCpacsCore.java:35)

This exception is coming from below method:

      private void  filterIneligibleCharges(ChargeProfileRequest request, ChargeProfile profile)
                                                                                                                        throws InvalidRequestException, EvaluationException
      {
            try{

            ChargeSet cs = null;
            ComplexCharge tmp = null;
            for (Iterator i = profile.iterator(); i.hasNext();)
            {      cs = (ChargeSet)i.next();
                  for (Iterator j = cs.iterator(); j.hasNext();)
                  {      tmp = (ComplexCharge)j.next();
                        //System.out.println("Checking if request is eligible for "+tmp.toString());
                        if (!tmp.isEligible(request))
                        {      //cs.removeCharge(tmp);
                              j.remove();
                        }
                  }
            }
      } catch(Exception e) {
           System.out.println("General Exception coming from ChargeProfileMapperImpl$$filterIneligibleCharges synchronized" + e);
           e.printStackTrace();
          }
      }


which is being called from one of public method as below:

    public ChargeProfile find(ChargeProfileRequest request,
                                              boolean applyEligibilityFilter) throws InvalidRequestException, DataMapperException
      {
            if (request.getDate() == null)
            {      throw new InvalidRequestException(
                        "The request must include a date for comparison against profile effective dates! ");
            }

            ChargeProfile profile = null;
            String cacheKey = toCacheKey(request);
            try
            {       if (!cache.containsKey(cacheKey))
                  {
                        profile = (ChargeProfile)abstractFind(request);
                        cache.put(cacheKey, profile);
                  }

                  else
                  {      cacheHit++;
                        profile = (ChargeProfile)cache.get(cacheKey);
                  }
                  if (profile != null)
                  {
                        profile.setInitialContext(request.getContext());
                        if (applyEligibilityFilter)
                        {
                              System.out.println("profile before filterIneligibleCharges " + profile);
                              filterIneligibleCharges(request, profile);
                              System.out.println("profile after filterIneligibleCharges " + profile);
                        }
                  }
            }
            catch (SQLException se)
            {      throw new DataMapperException("Exception finding charge profile for input:\n" + request, se);
            }
            catch (EvaluationException e)
            {      throw new DataMapperException("Exception evaluating charge eligibility for request:\n"
                        + request + "\n with unfiltered profile:\n" + profile, e);
            }
            catch (Exception e)
            {
                  System.out.println("ChargeProfileMapperImpl$$find " + e);
            }

            return profile;
      }


I am not sure what exactly is being modified with this method which is causing this issue to happen. Any ideas on how to look for it or fix it?

regards
Avatar of TheImmortal
TheImmortal
The java.util.ConcurrentModificationException indicated that you have an Iterator over an object. The Iterator is being walked through while something is trying to change the object upon which the iterator is based. I will try to look deeper in a little while. Maybe this will be enough to get you started.  Try commenting out the j.remove(), see if the ConcurrentMod exception goes away.
Avatar of aozarov
aozarov
Weird, that code looks ok.
Are you sure there no other threads in the system that modifies ChargeSet (adding/removing ComplexCharge)?
You can try doing:

cs = (ChargeSet)i.next();
synchronized (cs)
{
               for (Iterator j = cs.iterator(); j.hasNext();)
               {     tmp = (ComplexCharge)j.next();
                    //System.out.println("Checking if request is eligible for "+tmp.toString());
                    if (!tmp.isEligible(request))
                    {     //cs.removeCharge(tmp);
                         j.remove();
                    }
               }
}

But then you will need to do the same for every access to ChargeSet that might modify it or use a synchronized set instead (wrap it in Collections.synchronizedSet )
Avatar of Mick Barry
Mick Barry
Flag of Australia image
You need to synchronise that method or make changes elsewhere in your code so that it is only called from a single thread.
Avatar of TheImmortal
TheImmortal
The author stated that:

"synchronizing the methods is also a solution but I cannot do it because other task that I have been assigned to make this project live again is performance. If I synchronize the methods, I will lose performance"

The problem is that if this object is being shared among multiple threads in a non-synchronized manner, that these collisions will happen. Does each thread need to use the same object reference or can you Clone the object (or new Instance) to pass to each thread?
Avatar of Mick Barry
Mick Barry
Flag of Australia image
Thats why I mentioned that if they would need to change the code elsewhere to avoid synchronising.
Avatar of TheImmortal
TheImmortal
Performing the processing from a single thread would have pretty much the same effect as synchronizing the method. Especially if this is a service/interactive call upon which programs are awaiting the result. If this is simply a maintenance/background task call then a single thread should work.
Avatar of Mick Barry
Mick Barry
Flag of Australia image
I didn't necessariuly mean using a single thread. Cloning (as you mentioned) is another possible approach though there are again performance penalties there. Changing the design so the two threads don't share instances would also help.
Avatar of Mayank S
Mayank S
Flag of India image
>> If I synchronize the methods, I will lose performance.

Might still be better than cloning.
Avatar of narrav
narrav

ASKER

TheImmortal :

I tried commenting j.remove and looks like this exception is gone..

I understand synchronizing us a way to fix this issue, I already tried synchronizing this method, and this exception was gone.

But performance decreased.

Already performance for this project is worse.. it is right now 220 ms (without syncronizing on each request) and 350ms (after syncronizing).. and goal is to get 0.00002 ms. for each request.

Ofcourse code alone cannot do it.. so it might be hardware, java peformance, using cache etc.. that would be my other java question ..

but thats why i cannot use synchonize..


objects: I understand that some part of design/code has to be changed to allow multiple threads to use their own instance in this case.. but i am unable to figure out what object is being shared..

now we know that if I comment out j.remove -- problem is no more..

j is a local variable to this method on Chargeset Iterator, and ChargeSet is a local varilable to this method as well..  which we get from profile (ChargeProfile) iterator.. and chargeprofile is a local varialble in find method as below:

ChargeProfile profile = null;
profile = (ChargeProfile)abstractFind(request);

so why multiple threads are having problems?

any ideas how to fix it at design level rather than syncronizing or cloning (not sure how to use cloning -- sorry --never done it ..) but for this cloning might not be correct way..


any ideas pls
Avatar of aozarov
aozarov
>>  I comment out j.remove -- problem is no more..
Taking j.remove out means that you are not going to remove ComplexCharge from your ChargeSet, is this acceptable?

Calling a method with our without synchronization should not make much difference to your application.
The important part is that once you are synchronnizing on something other threads that are trying to aquire the same lock
will be blocked and become idle until you release it and by that you reduce the throuput of your application.

to clone a collection you just do:
(assuming ChargeSet extends some Collection API a.g. HashSet)

synchronized
cs = (ChargeSet)i.next();
ChargeSet clonedCs = null;

synchronized (cs)
{
clonedCs = (ChargeSet) cs.clone();
}

for (Iterator j = clonedSc.iterator(); j.hasNext();)
{     tmp = (ComplexCharge)j.next();
      //System.out.println("Checking if request is eligible for "+tmp.toString());
      if (!tmp.isEligible(request))
      {     //cs.removeCharge(tmp);
                         j.remove();
      }
}

For more about cloning see: http://www.churchillobjects.com/c/11091.html

Avatar of TheImmortal
TheImmortal
Cloning is basically where you create a *copy* of the shared object. This allows multiple threads to use the object without collission.   This of course means that each thread has its own instance of the object. Basically, when you commented out the j.remove(), the underlying object upon which the Iterator is based, ceased being changed. If the Object being used by the iterator is small enough, then you should be able to get away with making a new copy instead of sharing the reference among the threads.   Basically Here is what is happening:

Thread A                                           Thread B
  -> Get Object                                     -> Get Object
  -> Make Iterator                                 -> Make Iterator
   -> Loop Through Iterator                    -> Loop through iterator
     -> Remove Element                           XX Cannot complete loop because an element has been removed
                                                                  from the set upon which the iterator was built.

If the processing can be Thread independant, making a new instance ofo the data object may be the way to go. Otherwise, if you must share the object among threads, either do not remove the element as it is processed, or narrow the processing pipe (synchronized, single thread processing, etc).  

You could queue up the items to be removed, if that is totally neccessary.
Avatar of aozarov
aozarov
Ignore the "synchronized" keyword above cs = (ChargeSet)i.next();
Avatar of narrav
narrav

ASKER

I will try idea of cloning and see how it impacts the performance. I understand sycnrizning will not impact anything in the application, but will effect the throughput but througtput is a problem. As I said, I am trying not to degrade the performance of application and still make it multi-thread.

TheImmortal, I understand your explaination of why I am getting an error, but I am unable to understand, why in your example which object "Thread B"  has got which is equal to "Thread A". Thread B should have its own object and should work on its own object.. there is something being shared among 2 threads and thats what I need to find out and fix it.

>>If the processing can be Thread independant, making a new instance ofo the data object may be the way to go
what is this object .. thats what we are trying to find out.. I want to make new instance of this object, but not sure which object is this one..

>>Taking j.remove out means that you are not going to remove ComplexCharge from your ChargeSet, is this acceptable?
This is not acceptable.. I was just experimenting it per your suggestion..


ChargeSet doesn't extend anything..

Below is the constructor..

 public ChargeSet(Context context)
      {      this.context = context;
            charges = new ArrayList();
      }
      

Let me know if you need any more information, to figure this out more..

Thanks
Avatar of narrav
narrav

ASKER

oh well,

I cannot clone it because chargeset doesn't extend hashset is it???

any other way to clone?

and do you mean.. chargesset object is a problem?? why?
Avatar of TheImmortal
TheImmortal
If commenting out the j.remove(), then the object being shared is the Object upon which the Iterator is based.  As to why the error would occur, it would seem that two (or more) threads may be sharing the same reference to the base object (ChargeSet).  Looking closer, I think I see the problem. The ChargeSet seems to be obtains from the Profile Iterator (next). Becuase the ChargeSet is part of the Profile Iterator, performing the j.remove() is causingi the Object underneath the Profile iterator to change.  Since Profile is being passed in, either of those two points may be the problem.  
Avatar of aozarov
aozarov
>> for (Iterator j = cs.iterator(); j.hasNext();)
So ChargeSet implements the Iterator  and delegate the calls to the inner charges array?
You can make ChargeSet implements Cloneable
add a "public Object clone()" method and inside and do something like this:

public Object clone() throws ClonseNotSupportedException
{
ChargeSet cs = (ChargeSet) super.clone();
cs.charges = (ArrayList) charges.clone();
}

But if you want the the effect of removing ComplexCharge children from the ChargeSet to be visible outside of this method as well as to other threads then cloning is not the right solution for you.
Avatar of narrav
narrav

ASKER

TheImmortal,

But profile object is a local variable in find method which calls this "problem" method. So if the reference of profile object is passed from find method to this "problem" method, each reference should be a independant thread.. am i misunderstanding this?

aozarov : I am trying your idea now, and if it fixes it and what is the performance impact
Avatar of narrav
narrav

ASKER

aozarov : I didn't understood your comment of

>>But if you want the the effect of removing ComplexCharge children from the ChargeSet to be visible outside of this method as well as to other threads then cloning is not the right solution for you.

not sure exactly what you mean, but i don't want to change the functionality, i just want to make this process multi-thread
Avatar of aozarov
aozarov
>> and if it fixes it and what is the performance impact
What idea? synchronized, clone, ... ?
Clone should not be expensive operation as long as your ArrayList is not huge.
Avatar of TheImmortal
TheImmortal
Profile isn't local, as it is being passed in after being obtained from the abstractFind(request), this returns a reference to the object from wherever abstractFind is pulling it from.
Avatar of aozarov
aozarov
What I mean is that there should be some reason for that Iteration to remove those ComplexCharge from the ChargeSet.
If the effect of such removal is needed to be visible by others then cloning will not help you as you are modifying a local copy.
In that case you will need to synchronized the access to ChargeSet when you are updating its list or accessing its list.
Avatar of narrav
narrav

ASKER

aozarov :
while trying to implement your idea, I am getting below error

ChargeSet.java:258: clone() has protected access in java.lang.Object
    [javac]     cs.charges = (ArrayList) charges.clone();

where line 258:

cs.charges = (ArrayList) charges.clone();

and charges is a private instance variable.

anything i am doing wrong?

also should this method return anything?? as per your definition, it returns object, -- should i do a return of any object in this method
Avatar of narrav
narrav

ASKER

TheImmortal :


in find method i have :

ChargeProfile profile = null;
String cacheKey = toCacheKey(request);
try
 {       if (!cache.containsKey(cacheKey))
      {
            profile = (ChargeProfile)abstractFind(request);
            cache.put(cacheKey, profile);
      }
else
      {      cacheHit++;
            profile = (ChargeProfile)cache.get(cacheKey);
      }
      if (profile != null)
      {
      profile.setInitialContext(request.getContext());
      if (applyEligibilityFilter)
       {
            filterIneligibleCharges(request, profile);
      }


I am declaring ChargeProfile profile = null on top, isn't this local variable to find method???

Avatar of aozarov
aozarov
>> ChargeSet.java:258: clone() has protected access in java.lang.Object
Change charges declaration from List to ArrayList.

Sorry forgot the return value
public Object clone() throws ClonseNotSupportedException
{
ChargeSet cs = (ChargeSet) super.clone();
cs.charges = (ArrayList) charges.clone();
return cs;
}
Avatar of TheImmortal
TheImmortal
Thought the variable may be local, the scope of the object that is returned from the abstractFind (for each thread) could be the same; depending on how abstract find is creating the object being returned. If the object being returned is coming from a pool, then you are sharing the reference.  If the object being returned is being created for each request, then each thread should have its own instance.
Avatar of narrav
narrav

ASKER

aozarov :

while removing complexcharge from chargeset, one thread doesn't need to know other's object.. but profile for that object might be changed because:

If I am understanding the appl. correctly -- (sorry I am in process of understanding it as well.. just got this appl)

ChargeProfile is composed of different Contexts -- and each Context is composed of ChargeSet and each ChargeSet is composed of ComplexCharge.. so when we remove the complexCharge based on some algorithm from ChargetSet, it effects the Context and thus the profile..

Avatar of narrav
narrav

ASKER

TheImmortal:

ChargeProfileMapperImpl (with has these this find method and a "problem" method).. extends AbstractChargeMapper which extends AbstractMapper which has the implementation of abstractFind method as below:

protected Object abstractFind(Object key) throws SQLException
{
Object result = null;
Connection conn = null;
try
{      conn = DB.getConnection(MapperFactory.getDataSourceName());
      ResultSet rs = null;
      PreparedStatement findStatement = conn.prepareStatement(findStatement());

      loadFinderStatement(key, findStatement);
      rs = findStatement.executeQuery();
      if (rs.next())
      {      result = load(rs);
      }
      rs.close();
      DB.cleanUp(conn);
}
catch (SQLException e)
{
       throw new SQLException("Exception initialising mapper");
      //throw new SQLException("Exception initialising mapper!", e);
}
catch (Exception ex) {
}
return result;

}

so result is also a local variable.
Avatar of narrav
narrav

ASKER

sorry, I am still trying to run my program, I am running it on 10 threads, but for some reason, it succeeds sometimes, while at other times it just goes to sleep in between for no reason.. not sure what is happening which is breaking my program and it doesnot run further in between.. i know it is not a related question, but in case you have any ideas, pls let me know what to look for .. sometimes i get coredump.

below is the top output..

and my process is 2918


load averages:  0.07,  0.41,  1.18                                     22:32:23
1468 processes:1454 sleeping, 12 zombie, 1 stopped, 1 on cpu
CPU states: 96.1% idle,  0.5% user,  3.4% kernel,  0.0% iowait,  0.0% swap
Memory: 2048M real, 182M free, 87M swap in use, 1938M swap free

  PID USERNAME THR PRI NICE  SIZE   RES STATE   TIME    CPU COMMAND
 6618 rn73264    1  17    0 2704K 2072K cpu/1  12:44  1.70% top_5.6
  519 root       4  33    0    0K    0K sleep   6:48  0.12% snmpd
  798 jb85054    1   5    0 7952K 6088K sleep   2:05  0.10% kbgndwm
  296 root       6 -25    0 6384K 4456K sleep  33:11  0.08% automountd
  371 root       1  33    0 1064K  760K sleep   0:51  0.06% utmpd
 3869 jb85054    1  33    0 4240K 3808K sleep   0:12  0.06% aterm
 3400 jb85054    1  33    0 3096K 2392K sleep   0:06  0.02% ssh
24561 ps18394    8  34    0 9760K 8184K sleep   0:05  0.01% dtwm
12897 jb85054    1  34    0 1104K  800K sleep   0:00  0.01% tail
  530 root       1  33    0 4528K 2696K sleep   0:55  0.01% perl
16220 root       1  33    0 5256K 3264K sleep   0:09  0.01% sshd
29227 root       1  33    0 5096K 3240K sleep   0:16  0.00% sshd
 2161 bm99576    8  33    0 9768K 8160K sleep   0:05  0.00% dtwm
19994 mm90484    1  33    0   12M   11M sleep  13:14  0.00% emacs
 2918 rn73264   23  33    0  100M   64M sleep  11:21  0.00% java
 4059 ss44044    1  34    0   15M   13M sleep   4:00  0.00% xemacs-19.13
15451 root       1  33    0 5280K 3384K sleep   2:44  0.00% sshd
10236 rb13867    1  34    0   14M   12M sleep   2:43  0.00% xemacs-19.13
Avatar of Mick Barry
Mick Barry
Flag of Australia image
I'd think cloning would have a bigger impact on performance than synchronising would.
Avatar of aozarov
aozarov
>> sleep in between for no reason.. not sure what is happening which is breaking my program and it doesnot run further in between
Take a thread dump (kill -3 pid) to see what the application threads are doing at that point of time.
Avatar of narrav
narrav

ASKER

okay, will do..

also, will check the performance.

any ideas with my above explaination, what can be causing to make this exception happen at first place?

Avatar of Mick Barry
Mick Barry
Flag of Australia image
Your other problem sounds like a possible deadlock somewhere.
Avatar of narrav
narrav

ASKER

i am sorry objects.. could not understand .. what do you mean ..

i am asking ::

with above explaination that everywhere variable is declared as local variable .. then what can be the reason of having this exception ?? any ideas on it..  i understand ofcourse something is not right in the design which is messing the things up.. and thus  this is not a thread safe design.. and cloning or synch the method are patches which we can apply to work-around this..

but i want to understand the root-cause also that why is this happening.. what is the underline object/or reference to object that is being modified causing this scenario to happen..

I can send you guys the source code of some relevant classes in case you want to look at it.. by email.. but until now i am unable to figure out the root cause .. i want to change the design if possible.. if it is an easy change.. rather than sync'ing the method or cloning it..

please let me know if you can kindly help me figure out the root cause..

sorry, i am taking a look , and trying to understand.. i just need help so that i can understand better how design can mess up the things like in this case..
Avatar of narrav
narrav

ASKER

helo?? any ideas?
Avatar of narrav
narrav

ASKER

aozarov :

with clone implemented, I am having same exception but at other places:
Looks like we might have resolved this issue at this place, but at some other places, it exist as well, below is one of exception i am getting.

is there no way to find out underline objects easily?


java.util.ConcurrentModificationException
      at java.lang.Throwable.fillInStackTrace(Native Method)
      at java.lang.Throwable.fillInStackTrace(Compiled Code)
      at java.lang.Throwable.<init>(Compiled Code)
      at java.lang.Exception.<init>(Compiled Code)
      at java.lang.RuntimeException.<init>(Compiled Code)
      at java.util.ConcurrentModificationException.<init>(Compiled Code)
      at java.util.AbstractList$Itr.checkForComodification(Compiled Code)
      at java.util.AbstractList$Itr.next(Compiled Code)
      at com.citi.cititech.eae.util.object.ObjectPoolImpl.findPooledObjectByObject(Compiled Code)
      at com.util.DB.cleanUp(Compiled Code)
      at com.mapper.sybase.AbstractMapper.abstractFind(Compiled Code)
      at com.mapper.sybase.DynamicDataMapperImpl.find(Compiled Code)
      at com.mapper.sybase.DynamicDataMapperImpl.findMap(Compiled Code)
      at com.mapper.sybase.GeneralProfileMapperImpl.mergeDynamicData(Compiled Code)
      at com.mapper.sybase.GeneralProfileMapperImpl.find(Compiled Code)
      at com.ClientProfileService.getGeneralProfile(Compiled Code)
      at com.ccs.ChargeCalculationService.applyCharges(Compiled Code)
      at com.ccs.ChargeCalculationService.doCalculation(Compiled Code)
      at com.test.TestDriver.testCalculation(Compiled Code)
      at com.test.ThreadCpacsCore.run(Compiled Code)


SOLUTION
Avatar of TheImmortal
TheImmortal
Link to home
membership
This solution is only available to members.
To access this solution, you must be a member of Experts Exchange.
Start Free Trial
Avatar of aozarov
aozarov
>> is there no way to find out underline objects easily?
You have two solutions.
1. to synchronized your access to the collections (java.util.ConcurrentModificationException is only one symptom. if you are using non synchronized collections such as
LinkedList any read access that runs concurrently to write access is bad).
2. Clone your collection when updating it (so basically you are updating a copy). most of the times after you update the copy you want to replace the orignal with the copy
so to make those updates visible to others.

You have solutions here for the two aproaches and you will need to implement them any place you have a concurrent read/write access for those collections.
Avatar of narrav
narrav

ASKER

TheImmortal:

You were very correct with having problem in the cleanUp.

below is the method :

public static void cleanUp(Connection conn) throws SQLException
{      try{
System.out.println("Working on conn = " + conn);
if (conn != null)
{      IObjectPool pool = null;
IPooledObject pooledConnection = null;
for (Iterator i = pools.values().iterator(); i.hasNext();)
{      pool = (IObjectPool)i.next();
pooledConnection = pool.findPooledObjectByObject(conn);
if (pooledConnection != null)
{      pooledConnection.release();
// found the right connection, so get out of here!
break;
}
}
}
} catch(Exception ex) {
System.out.println("DB$$cleanUp - General Exception while trying to cleanUp conn " + ex + " while working on conn = " + conn);
ex.printStackTrace();
}
}


and I was able to get this GeneralException.  (I put this try-catch here.. it was not there..and so I was not sure where is this coming from).

But after I synchronized call to this method, still I am getting this exception.

This method is supposed to cleanUp the conn's  to give back to pool.. well, this is third party tool, and only class I have is DB.java which I can send it out if you want to look at it..

but synchronizing cleanUp is not working for me in this case.. any ideas?

aozarov : Thanks for explaining the solutions..
I understood mostly what u mean, but what I don't understand is -- why should I synchronize the methods to make it thread-safe..I want to find out underline objects and make them local variable. but problem is finding them.. I can sychronize DB.cleanUp just becoz atleast it not delaying my response to the client, it is just meant for local purpose to get the conn back to pool, but I don't want to synchronize the first "problem" method because response is build based on that..

but not sure why sync'ing DB.cleanUp is not working either..

any ideas how to fix this.. Ofcourse if multi-thread is a problem with 3rd party tool for any reason-- we cannot do much, but 2 things
1). I want my project to be multi-thread without synchronzing or cloning any methods which impacts response.
2). I want to find a valid reason justifying that this is a problem because of 3rd parry component.. and so we cannot fix it..

thats what i am trying to accomplish

thanks
Avatar of narrav
narrav

ASKER

BTW,

pool is an class varialble defined as below:
private static Map pools = new HashMap();
Avatar of aozarov
aozarov
>> why should I synchronize the methods to make it thread-safe..I want to find out underline objects and make them local variable
First you don't have to synchronized the methods. You need to synchronized the access to those non thread safe objects that are being access/manipulated concurrently (e.g. ArrayList).
That can be done by sycnrhonized the method of the non thread safe objects or by synchronizing the access to them (e.g. in the case of iteration). As I said you can't avoid that unless
you are making a local copy (the process of making the local copy should be synchronized as well) and then working on the copy. The problem with the copy aproach is that updates to the copy
will not be reflected when using the original (something that maybe needed). This is where you want to replace the orignal with the copy.
Avatar of aozarov
aozarov
>> But after I synchronized call to this method, still I am getting this exception.
That is because someone else is updating the pools.
When using HashMap (which synchronized its access to the methods) you can do this (instead of synchronizing the method):
synchronized(pools)
{
for (Iterator i = pools.values().iterator(); i.hasNext();)
{     pool = (IObjectPool)i.next();
pooledConnection = pool.findPooledObjectByObject(conn);
if (pooledConnection != null)
{     pooledConnection.release();
// found the right connection, so get out of here!
break;
}
}
}
Avatar of narrav
narrav

ASKER

I tried

sycnchronized(pools)...

it didn't work

but when i did a synchrnozed(pool) it seems to be working

public static  void cleanUp(Connection conn) throws SQLException
{      try{
System.out.println("Working on conn after synchronizing pool only = " + conn);
if (conn != null)
{      IObjectPool pool = null;
IPooledObject pooledConnection = null;

for (Iterator i = pools.values().iterator(); i.hasNext();)
{      pool = (IObjectPool)i.next();
synchronized(pool) {
pooledConnection = pool.findPooledObjectByObject(conn);
if (pooledConnection != null)
{      pooledConnection.release();
// found the right connection, so get out of here!
break;
}
}

}

}
} catch(Exception ex) {
System.out.println("DB$$cleanUp - General Exception while trying to cleanUp conn " + ex + " while working on conn = " + conn);
ex.printStackTrace();
}
}


is this the solution??

aozarov : I udnerstood your comment but not completely,

I want you to make a local copy and then work on a copy ..(especially with my 1st exception) or atleast find out which iterator arraylist is giving me problem.. but i could not find it..

Lets take my 1st exception as an example -- is there a way to find out what object is giving us problem??

can u help me locate that.. looks like i am failing to find that

2nd exception is cleaning up conn .. so i don't care much on performance i guess ..
but still i am not able to get why i have to sychronize pool object to get rid of this problem while this is a local variable clearly declared in this cleanUp method as

IObjectPool pool = null;

sorry i am confused!!
Avatar of aozarov
aozarov
>> not able to get why i have to sychronize pool object to get rid of this problem while this is a local variable clearly declared in this cleanUp
Though pool is a local variable it points to some instance (given by the calls pools.values().iterator(),  i.hasNext(),  pool = (IObjectPool)i.next() )
That instance is used elsewhere and while you were using it (findPooledObjectByObject) some other thread used it as well.
If for example you were deep cloning *that instance* then you could use it safetly.

Regrading connection leak, make sure you always close your connections in a finally clause.
e.g.
Connection c= ...
try
{
get c
do work with c
}
finally
{
     if (c != null)
       c.close();
}
Avatar of narrav
narrav

ASKER

aozarov:
Undertood your first comment..
also, understood conn leak thing.. thanks!! though not sure .. but i didn't ask this question..

but anyway, thanks .. every knowledge from experts helps me ..

i still need help with my first exception because i don't want to use cloning on chargeset..i want to find arraylist or object that is having problem and make that as a local variable instead.. would really appreciate if you can help with that..

here is the flow..

my ThreadCode makes many instances of TestDriver and TestDriver program creates instance of CCS Object (singleton) and CCS getInstance creates instance of CPS Object (singelton),  takes input from a file (based on each line ) each line is a request and call ccs.doCalculation method .

ccs.doCalcuation calls
Map charges = cps.getProfiledCharges(req,....)

cps.getProfiledCharges has a local variable:
ChargeProfile profile = chargeProfileMapper.find(req,true);
  where note that chargeProfileMapper is an instance variable of CPS .. (so threads are sharing it).

chargeProfileMapper.find method :

This chargeProfileMapper in an interface --- so this call to find method goes to chargeProfileMapperImpl.
where

chargeProfileMapperImpl extends AbstractChargeMapper ----    AbstractChargeMapper extends AbstractMapper

chargeProfileMapperImpl find method
declares
ChargeProfile profile=null;
profile=(ChargeProfile)abstractFind(req)

and at some point in this method it calls

if(applyEligiblityFilter)
 filterInEligibleCharges(request,profile)  -- which we have issue with.

call to abstractFind goes to AbstractMapper where this method is implemented returns Object

this Object is defined as local variable in abstractFind method as below:

Object result=null;

result=load(rs);  where rs is a resultset based on query .. this rs is again declared as local var in abstractFind method as below:

Resultset rs = null;

PreparedStatement findStatement = null;
findStatement = conn.prepareStatement(findStatement());

rs=findStatement.executeQuery();

load method which actually loads the Object -- calls as below:

protected Object load(ResultSet rs) throws SQLException
      {      Object key = createKey(rs);
            Object result = doLoad(key, rs);
            return result;
      }

and doLoad method is an abstract Method in this class and thus goes back to child -- chargeProfileImpl -- implementation as below:

 protected Object doLoad(Object key, ResultSet rs) throws SQLException
      {      ChargeProfile prof = new ChargeProfile();
            Map setsSoFar = new HashMap();
            Map chargesSoFar = new HashMap();

            Long chargeId = new Long(rs.getLong("charge_id"));
            ChargeSet set = new ChargeSet((Context)key);
            setsSoFar.put(set.getContext(), set);
            prof.addChargeSet(set);

            ComplexCharge tmp = processCharge(chargeId, rs);

            set.addCharge(tmp);
            chargesSoFar.put(chargeId, tmp);

            Context context = null;
            while (rs.next())
            {
                  context = (Context)createKey(rs);
                  chargeId = new Long(rs.getLong("charge_id"));
                  if (setsSoFar.containsKey(context))
                  {      set = (ChargeSet)setsSoFar.get(context);
                        if (chargesSoFar.containsKey(chargeId))
                        {      tmp = (ComplexCharge)chargesSoFar.get(chargeId);
                              tmp.addTier(processTier(rs));
                        }
                        else
                        {
                              tmp = processCharge(chargeId, rs);
                              set.addCharge(tmp);
                              chargesSoFar.put(chargeId, tmp);
                        }
                  }
                  else
                  {      set = new ChargeSet(context);
                        setsSoFar.put(context, set);
                        prof.addChargeSet(set);
                        tmp = processCharge(chargeId, rs);
                        set.addCharge(tmp);
                        chargesSoFar.put(chargeId, tmp);
                  }
            }
            return prof;
      }

Thus this profile object to casted to ChargeProfile in chargeProfileImpl and passed to filterIneligibleCharges ..

anything u see faulty in this whole flow causing this issue to happen.....


Thanks alot.. I really appreciate all the help i am getting here

           
Avatar of aozarov
aozarov
>> (ChargeProfile)cache.get(cacheKey);
I think in your case the simplest/safest aproach whould be to do a deep clone to ChargeProfile when you get it from the cache.
see this link for how to: http://www.churchillobjects.com/c/11091d.html
deep cloning would probably much faster then getting from the DB (if you were to avoid cache) and you will need to do one and only here.
This way different find class will not mess each other ChargeProfile local objects.


Avatar of narrav
narrav

ASKER

aozarov:

thanks for the link on deep cloning.. i am looking at it to understand it better..

but well, i have been told by management not to use cloning or synchronization. rather than fix the design unless this design cannot be fixed for some business reason. ofcourse they dont want me to change the design, but fix it by changing scope of variables

i have to provide them the flow and explain why this issue can be fixed only by cloning and not by changing some variable. I have to list all the steps to make this project multi-thread. so somehow i have to do it widout cloning or synch.. perferably unless this is impossible in which case i have to understand and porvide them the reason.

Anything u can help me with this knowing this flow.. i can provide more info if u want me to in case it helps u..
ASKER CERTIFIED SOLUTION
Avatar of aozarov
aozarov
Link to home
membership
This solution is only available to members.
To access this solution, you must be a member of Experts Exchange.
Start Free Trial
Avatar of narrav
narrav

ASKER

aozarov
I am almost at the point of understanding exactly your explaination what can be happening.

Before I write down what exacly I have understood from your above comment, let me ask you this,

Though you are correct, we are using caching mechanism to retreive profile from cache, but ideally in this project this implementation is not complete.

and hence ideally we should not be using cache at all for this function (for this find function i mean).

To achive this, in our config file, we have defined expiry_time = 1 (i guess it is 1 ms.. not sure about the UNIT of time here.. ) as below

<node name="CACHE">
        <map>
          <!-- Default Cache settings for static data including product,
               account and profile lookup results -->
          <entry key="ImplClassName"
          value="com.citi.cititech.eae.services.app.cache.oscache.OSCacheService
Impl" />
          <!--<entry key="caching_strategy" value="UNLIMITED" /> -->
          <!-- expiry_time is based on ms, so a value of 300000 keeps data in ca
che for at most five minutes
               if the strategy is LRU.  The expiry time is based on either ACCES
S_TIME or CREATION_TIME -->
          <entry key="caching_strategy" value="LRU" />
          <entry key="cache_size" value="1" />
          <entry key="expiry_time" value="1" />
          <entry key="expiry_based_on" value="CREATION_TIME" />
        </map>

so ideally we should not be using cache for this thing..

so what do you think ?? do u still think problem is coming from cache ???

Avatar of narrav
narrav

ASKER

Also,
while I am trying to understand your argument of cache, if I assume that cache is being used here may be because before it expires, 2 threads picks up the cache and thus the same instance of chargeprofile .. but still having a confusion ..

in find method, we have defined a local var as

ChargeProfile profile = nulll;

and then we do

profile = (ChargeProfile)cache.get(cacheKey)..

so profile is a local var -- and each thread should have it own local instance of profile..

cache will be shared among threads .. but the profile instance.. then even if cache is used.. why should this issue come up..

Avatar of aozarov
aozarov
>> so what do you think ?? do u still think problem is coming from cache ???
I tend to believe so, otherwise the find will create a new instance of ChargeProfile and its internal elements and that should not be shared among different Threads/Requests.
Try to set the cache to: <entry key="cache_size" value="0" />

>> so profile is a local var -- and each thread should have it own local instance of profile..
No, each threads uses a local variable that points to/reference the same instance. (their share/point to the same copy)
Avatar of narrav
narrav

ASKER

ok,

I am trying with setting cache_size = 0 and lets see if problem still persists..

in the mean-time, i am also trying to implement deep cloning method -- which you explained.. only thing is -- i am trying to do it on ChargeProfile level instead of ChargeSet level.. because profile is the one being modified..

i implemented clone method in ChargeProfile as the way you told me..

but for putting the deep cloning, i guess i have to do something like this:

public Object clone() throws CloneNotSupportedException
      {
      System.out.println("in ChargeProfile$$clone");
      Object cloneCp = null;
      cloneCP = super.clone();
      cloneCp.chargeSets = (ArrayList) chargeSets.clone();
      
      
      ((ChargeProfile)cloneCp).setChargeSet((ArrayList)chargeSets.clone());
      
      return cloneCp;
      }


so -- do i need to implement clone at ChargeSet level also?

and I don't have any method in ChargeProfile to setChargeSet arraylist. I only have a add method for this:


      public List getChargeSets()
      {      return chargeSets;
      }

      /**
     * @return An iterator over the ChargeSet collection.
     */
    public Iterator iterator()
      {      return chargeSets.iterator();
      }

      /**
     * Appends the specified element to the end of this list.
     * @param cs - A ChargeSet.
     * @return true (as per the general contract of Collection.add).
     */
    public boolean addChargeSet(ChargeSet cs)
      {      return chargeSets.add(cs);
      }

so how should i implement deep cloning??
Avatar of aozarov
aozarov
>> so -- do i need to implement clone at ChargeSet level also?
Yes, because you are manipulating it by removing ComplexCharge from its list.

>> so how should i implement deep cloning??
public Object clone() throws CloneNotSupportedException
     {
     System.out.println("in ChargeProfile$$clone");
     Object cloneCp = null;
     cloneCP = super.clone();
     cloneCp.chargeSets = new ArrayList(chargeSets.size());
     
     for (Iterator i =  chargeSets.iterator(); i.hasNext(); )
     {
               ChargeSet charegeSet = (ChargeSet) i.next();
               cloneCp.chargeSets.add(charegeSet.clone()); // You will need to have a clone in chargeSet method that internally clones its ComplexCharge array list
     }

     return cloneCp;
     }
Avatar of narrav
narrav

ASKER

got it aozarov:

just a question,

if i just clone chargeset -- like the way you told me before, it also fixes the problem..  but I am not able to understand why it fixes my problem while actually ChareProfile is being modified and not the ChargeSet.. any ideas?

rather above way of cloning the chargeset should be incorrect because--
ChareProfile has list of ChargeSet and Chargeset has list of Charges.. so if we clone clone Chargeset and modify it.. this cloned Chargeset is not actually tied up with profile -- not sure if profile would really be modified or not....   ??

I have implemented deep cloning on ChargeProfile the way you described..

actually this is what I do as below:

In ChargeProfile, I have a method:

public Object clone() throws CloneNotSupportedException
{
System.out.println("in ChargeProfile$$clone");
ChargeProfile cloneCp = null;
cloneCp = (ChargeProfile)super.clone();
cloneCp.chargeSets = new ArrayList(chargeSets.size());

for (Iterator i =  chargeSets.iterator(); i.hasNext(); )
{
ChargeSet chargeSet = (ChargeSet) i.next();
cloneCp.chargeSets.add(chargeSet.clone());
// we need to have a clone in chargeSet method that internally clones its ComplexCharge array list
}

return cloneCp;
}

and in ChargeSet -- clone method as below:


public Object clone() throws CloneNotSupportedException
{
System.out.println("in ChargeSet$$clone");
ChargeSet cs = (ChargeSet) super.clone();
cs.charges = (ArrayList) charges.clone();
return cs;
}


and in find method in ChargeProfileImpl,
I have

if (profile != null)
{
ChargeProfile clonedProfile = null;
clonedProfile = (ChargeProfile)profile.clone();
profile.setInitialContext(request.getContext());
if (applyEligibilityFilter)
{
System.out.println("profile before filterIneligibleCharges " + profile);
System.out.println("ChargeProfile from cache  " + (ChargeProfile)cache.get(cacheKey));
//filterIneligibleCharges(request, profile);

profile = filterIneligibleCharges(request, clonedProfile);

System.out.println("profile after filterIneligibleCharges " + profile);
}


Note :  I have another object clonedProfile -- (clone of original profile) which I am passing to filterIneligibleCharges.

filterIneligibleCharges -- I have changed to return profile instead of void.

is this correct??

also, one more question,

without all these changes above, filterIneligibleCharges method was changing the reference of profile object - which points to actual object in cache right?? So bascially cache was being changed..

but with all these clone changes -- cache will not be changed right??

Thanks

Avatar of aozarov
aozarov
The code looks good now.

>> so if we clone clone Chargeset and modify it.. this cloned Chargeset is not actually tied up with profile
Right, you don't want to do that.

>> filterIneligibleCharges -- I have changed to return profile instead of void.
You can keep it as is (void) as you pass it the cloned object (so that method will change this clone).

>> above, filterIneligibleCharges method was changing the reference of profile object - which points to actual object in cache right?? So bascially cache was being changed..
Right, without the changes the ChargeProfile which is stored in your cache will be changed.

>> but with all these clone changes -- cache will not be changed right??
Right, because you are chaning the deep cloned one instead.
Avatar of narrav
narrav

ASKER

>> filterIneligibleCharges -- I have changed to return profile instead of void.
>>You can keep it as is (void) as you pass it the cloned object (so that method will change this clone).

but what if I want to change the cache as well as the way it was implemented before..?

also, do you mean that
find method in ChargeProfileMapperImpl should return clonedProfile instead of profile..??

one more question:
I was studying the URL you sent me about deep and shallow cloning..

it says that :: >>clone will contain the copy of the same reference pointing to the same Object ..
so isn't clone just another reference to actual object?
Avatar of aozarov
aozarov
>> but what if I want to change the cache as well as the way it was implemented before..?
Then use the object returned from the cache instead of the cloned one (but here you will have the same access problem).
What you can also do is once you completed the updates to the cloned to replace the cahced one with it.

>> find method in ChargeProfileMapperImpl should return clonedProfile instead of profile..??
Right.

>> so isn't clone just another reference to actual object?
Right, but this is why you do deep clone.
Avatar of narrav
narrav

ASKER

hmm,

>> but what if I want to change the cache as well as the way it was implemented before..?
>>Then use the object returned from the cache instead of the cloned one (but here you will have the same access problem).
What you can also do is once you completed the updates to the cloned to replace the cahced one with it.

I got it, but then isn't it better the way I doing.. to change filterIneligibleCharges to return profile, and then in find method, do something like:

profile = filterIneligibleCharges(request, clonedProfile);

where I guess profile is a reference pointing to cache object, and this cache object will be replaced (modified) and since I am passing clonedProfile to filterIneligibleCharges, so my problem of concurrent modification shud be gone..

>>>> so isn't clone just another reference to actual object?
>>Right, but this is why you do deep clone.

well, what I understood from deep clone is .. it is just cloning the inside objects..

like in our example --

we clone ChargeProfile, and in clone method of ChargeProfile, we clone ChargeSet.

but cloining ChargeProfile -- (since it is a reference) it still pointing to profile (which is still poiniting to Cache Object)..

so how is deep cloning taking care of it? same it with ChargeSet -- we just created a new reference of clonedChargeSet..

how it is resolving the problem?


Avatar of aozarov
aozarov
>> profile = filterIneligibleCharges(request, clonedProfile);
>> where I guess profile is a reference pointing to cache object, and this cache object will be replaced (modified)
No, the cache object will not be replaced.
when you do: XXX profile = cache.get(zzz);
You basically declare a local variable "profile" that points to the same instance/place the cache is pointing.
If later on you change it (do profle = filterIneligibleCharges(..)) you basically change where the local variab le profile is pointing
but this does not change the object which the cache is pointing to.
To do that you will need to update the cache by doing something similar to cache.put(key, profile)

>> but cloining ChargeProfile -- (since it is a reference) it still pointing to profile (which is still poiniting to Cache Object)..
You deep clone the value returned from the cache and asign it to the lcoal profile variable.
Then once the update is completed you can put back the cloned object (pointed by profile to the cache -> cache.set(key, profile)).
You do that only if you want to update the cache with the updated/filtered profile. (I don't think you have any reason to do it though).

>> how it is resolving the problem?
For each request the local variable profile will point to a *NEW* cloned instance so changes will not effect other requests.
Avatar of narrav
narrav

ASKER

aozarov :

thanks alot for all the help,

I am in process of testing the code after making cloning changes to see if everything is working as expected.

I am getting some errors (Null pointer's etc) while running this program.. not sure it is because of cloning for some reason or some place else.

since I am running 10 threads, and system.out goes to stdout .. and error is going on stderr everything is coming out to be clobbered to really see what thread is giving problem and on what request..

is there a way to separate stdout and stderr for each thread.. and merge stdout and stderr to see what was the request and what stacktrace it generated..

right now, request goes to system.out
and in exception block, code has e.printstacktrace.. so i see exceptions coming up, but i am not able to relate the system.out with exceptions and since it is coming from 10threads in same stdout, it is difficult to really see from which thread is it coming from..

i know implementation of logs should be a good idea.. but i don't want to do it at this moment as i have been told to quickly fix this project for multi-thread and then team would be hired to further refine this project..

any ideas,

thanks
Avatar of aozarov
aozarov
You can redirect stderr to the same place as stdout by adding 2>&1 to your java command line.
e.g java -classpath . MyTest 2>&1
This should work for both unix and windows.

>> it is difficult to really see from which thread is it coming from..
Add to the System.our.println a prefix which will be assigned to Thread.currentThread().getName() [this will have a unique string per thread]
Avatar of narrav
narrav

ASKER

Thanks aozarov,

>> it is difficult to really see from which thread is it coming from..
>>Add to the System.our.println a prefix which will be assigned to Thread.currentThread().getName() [this will have a unique string per thread]

I thought there might be some other easy to do it since changing System.out.println in the whole code is a night-mare.
Is there any wrapper i can use which does this??


also, since there is so many things in this project, i really need to mnake a data flow for this project which basically describes each class in the project, its instance, local variables, methods and description what they are doing.. and then the flow -- and i want to do it on one page (huge chart kind of thing).. which I can present to other people who will be joining the team..

any tool u can suggest which can i do this easily.. i started this in visio, but i don't think i can import the whole project in visio .. i want something which generate for me mostly all the stuff, and then i can just cross-check and fill the extra things if any..

any ideas which can help me generate this chart quickly.. ??


Avatar of aozarov
aozarov
>> Is there any wrapper i can use which does this??
I assume you don't want to use log4j (which will do this job for you)
You can do something like this (at the begining of your program - like the main method):
System.setOut(new java.io.PrintStream(System.out)
{
public void println(String str)
{
       super.println(Thread.currentThread().getName() + ": " + str);
}
});

>> any ideas which can help me generate this chart quickly.. ??
You can use some UML editor (which can import your java files and create a UML diagram) like arguUML (see: http://argouml.tigris.org/ )
Avatar of narrav
narrav

ASKER

I want to use log4j, but I don't want to do it right now since I don't want to change the code for this .. I just want to make it multithread quickly and with least changes. implementing log4j is another whole project.. right now we only have system.out.println..
I will use your way, hopefully it will have less changes..


Thanks for the UML Editor,

but if I want to present this chart to other people, they will have to have same software to open it right? or is there a better way to send it.. or save it some other software which people usually have?
Avatar of TheImmortal
TheImmortal
Usually, You can same the UML to images (JPEG, BMP, etc). Most software allows you to export the entire UML project to a "javadoc" like format.
Avatar of aozarov
aozarov
>> they will have to have same software to open it right?
As TheImmortal suggests argoUML can export it to an image.
Avatar of narrav
narrav

ASKER

aozarov:

You method for STDOUT is working out great,

Just one thing, like the way i am able to get ThreadName and string and STDOUT... is there a way to generate different files of STDOUT for different names (may be based on ThreadName)..

and how can i do same thing for STDERR?
Avatar of aozarov
aozarov
>> and how can i do same thing for STDERR?
Do the same for for:
System.setErr(new PrintStream(System.err)
....

>> is there a way to generate different files of STDOUT for different names
For that you need the provideded PrintStream wrapper smarter and to append to a different file based on that thread name.
Just more logic inside the above.
Avatar of narrav
narrav

ASKER

aozarov :

Looking into the UML editor u sent, looks like it doesn't create a data flow.. I guess I want a chart for this project - which shows inheritance, class diagram as well as a data flow in one shot, this tool doesn't do that ..

any other strong tool if u know..??
Avatar of TheImmortal
TheImmortal
If you have ever used Eclipse, there is a UML plugin for that.  Borland makes a nice UML editor (formerly Together J),  Jude, Poseidon UML is nice. There are tons of them out there.
Avatar of narrav
narrav

ASKER

TheImmortal,

thanks!! I am trying to get Rational Rose, do u think it will help?

also, with your code snippet, it looks interesting. thanks!! but not sure how should i generate different filenames based on threadname..  and do a system.out.println in that file.. looks like as said aozarov, it needs more logic. not sure what exactly would be the logic, any ideas?
Avatar of aozarov
aozarov
>>not sure what exactly would be the logic, any ideas?
Have a map of FileoutputStream.
then based on the thread name get from the map (if not exists then create a file based on the thread name and add to the map).
and write to that file outputstream
Avatar of TheImmortal
TheImmortal
I used Rational a long time back (mid 90s), it is a decent (but expensive) tool. It does a lot of reverse and forward engineering.

As for the logic, you would basically use the thread name as a basis for the file name. For example:

  public static void main(String args[]) {

  PrintStream ps=null;
  BufferedOutputStream bos=null;
  FileOutputStream fos=null;

  for (int x=0;x<5;x++) {

   if (null!=ps) {
      ps.flush();
     try {
      ps.close();
      } catch (Exception e) {}
    }

  try {
  fos=new FileOutputStream("output"+x+".dat");
  bos = new BufferedOutputStream(fos);
  ps=new PrintStream(bos);
   

   System.setOut(ps);
   } catch (Exception e) {}

   for (int j=0;j<10;j++) {
    System.out.println("Some Data line "+j);
   }
   }

   if (null!=ps) {
      ps.flush();
     try {
      ps.close();
      } catch (Exception e) {}
    }


  }

Notice I am not using the anonymous creation. Since the anonymous creation can lead to a build up of file descriptors, it am bad :)  So I create them, the close the PrintStream which should close the rest of the child streams when you change to a new Thread or something. Remember to flush the output stream to get all the data written to the file.
Avatar of narrav
narrav

ASKER

/*
 * Created on Apr 11, 2005
 *
 * TODO To change the template for this generated file go to
 * Window - Preferences - Java - Code Style - Code Templates
 */
package com.citigroup.cpacs.test;

import java.util.Hashtable;
import java.util.ArrayList;
import com.citigroup.cpacs.test.utils.StringExt;
import java.io.BufferedReader;
import java.io.FileReader;
import java.io.IOException;
import java.io.*;




/**
 * @author ms97167
 *
 * TODO To change the template for this generated type comment go to
 * Window - Preferences - Java - Code Style - Code Templates
 */




public class MultiThreadCpacsCoreTest {

      public static void main(String[] args) throws Exception
      {

/*        System.setOut(
            new PrintStream(
              new BufferedOutputStream(
                new FileOutputStream(Thread.currentThread().getName()+"output.dat")))



        //System.setOut(new java.io.PrintStream(System.out)
        {
        public void println(String str)
        {
               super.println(Thread.currentThread().getName() + ": " + str);
        }
 });
*/


       fileName = args[0];
       System.out.println("FileName is: " + fileName);
       MultiThreadCpacsCoreTest m = new MultiThreadCpacsCoreTest();
       m.executeFile();
      }


 public void executeFile() throws IOException {

           PrintStream ps=null;
           BufferedOutputStream bos=null;
         FileOutputStream fos=null;

           if (this.fileName.trim() == "") {
              throw new IOException("File name not been set!.");
           }

           BufferedReader reader = new BufferedReader(new FileReader(this.fileName));
           if (reader ==null) {
              System.out.println("No file to read");
           }

           String sLine = reader.readLine();
           Hashtable  ht = new Hashtable();
           ArrayList threadCpacsCore=new ArrayList();
           while (sLine != null) {
                System.out.println("Processing line:" + sLine);
                String asFieldValues[] = StringExt.split(sLine,",");
                // break each pair into its components and build a map
                for (int inCont=0; inCont < asFieldValues.length; inCont++) {
                  String lsPairvalues[] = StringExt.split(asFieldValues[inCont],"=");
                  if (lsPairvalues.length ==2) {
                     ht.put(lsPairvalues[0],lsPairvalues[1]);

                  }

                }

            ThreadCpacsCore tpc = new ThreadCpacsCore(ht.get("ThreadName").toString(),ht.get("InputFile").toString(), ht.get("OutputFile").toString(),ht.get("RunOption").toString());
            threadCpacsCore.add(tpc);
            ht.clear();

                System.out.println("===========================================");

                sLine = reader.readLine();
           }


           reader.close();





         for(int index = 0;index < threadCpacsCore.size();index++) {
               ThreadCpacsCore tcp = (ThreadCpacsCore) threadCpacsCore.get(index);

               if (null!=ps) {
                                     ps.flush();
                                    try {
                                     ps.close();
                                     } catch (Exception e) {}
                             }


                             try {
                                               fos=new FileOutputStream("Thread"+index+".dat");
                                               bos = new BufferedOutputStream(fos);
                                        ps=new PrintStream(bos);

                               System.setOut(new java.io.PrintStream(bos)
                               {
                             public void println(String str)
                               {
                              super.println(Thread.currentThread().getName() + ": " + str);
                              }
                          });

            } catch (Exception e) {}




               tcp.start();
          }



          if (null!=ps) {
                    ps.flush();
                   try {
                    ps.close();
           } catch (Exception e) {}

         }

}
       private static String fileName = "";

}

for some reason, when i run it,

it is stopping, it basically reads a input file in which i define number of threads (example as below)
ThreadName=Thread1,InputFile=inputMasterFile.txt,OutputFile=MultiThreadCodeFiveThread-1.csv,RunOption=ALL
ThreadName=Thread2,InputFile=inputMasterFile.txt,OutputFile=MultiThreadCodeFiveThread-2.csv,RunOption=ALL
ThreadName=Thread3,InputFile=inputMasterFile.txt,OutputFile=MultiThreadCodeFiveThread-3.csv,RunOption=ALL
ThreadName=Thread4,InputFile=inputMasterFile.txt,OutputFile=MultiThreadCodeFiveThread-4.csv,RunOption=ALL
ThreadName=Thread5,InputFile=inputMasterFile.txt,OutputFile=MultiThreadCodeFiveThread-5.csv,RunOption=ALL


and thus i need 5 stdout files ..

i see 5 dat files .. but only some are being written with value, rest remains empty and finally program exits looks like with no errors..

anything i am doing wrong in the code?

Avatar of TheImmortal
TheImmortal
Well, one thing I see is this:

ps=new PrintStream(bos);

                            System.setOut(new java.io.PrintStream(bos)

You don't need to make a new PrintStream since you just made one. Do this instead:

System.setOut(ps);

Not othe problem, but one of the reasons why your logs aren't flushing.
Avatar of narrav
narrav

ASKER

not sure,

but when I do
 System.setOut((ps)
                               {
                             public void println(String str)
                               {
                              super.println(Thread.currentThread().getName() + ": " + str);
                              }
                          });

I get compilation errors ..

java:122: ')' expected.
                                     System.setOut((ps)
                                                       ^
MultiThreadCpacsCoreTest.java:123: '}' expected.
                                     {
                                      ^
MultiThreadCpacsCoreTest.java:124: Statement expected.
                               public void println(String str)
                               ^
MultiThreadCpacsCoreTest.java:128: Class or interface declaration expected.
                          });



how shud i write a println method in this case?
Avatar of TheImmortal
TheImmortal
because you have malformed syntax. Just do:

System.setOut(ps);

You don't need the curly braces.
Avatar of narrav
narrav

ASKER

But this way how will each thread write to its own file without the println method???

I thought we are overriding the println method .. so that each thread writes STDOUT to its own specific file.
Avatar of TheImmortal
TheImmortal
We are overriding the location to which System.out.println writes data.   I am not sure where the disconnect is, so I will try to explain this again:

- Each thread will override the STDOUT with the method we have already established ( BufferedOutputStream blah blah blah System.setOut(ps) blah blah blah)

- Each thread will do System.out.println("whatever data to be printed out")

Does that make it clearer?
Avatar of aozarov
aozarov
You could have done something simple as that (as I suggested above) [didn't check if it compiles]:

System.setOut(new java.io.PrintStream(System.out)
{
Map files = new HashMap();

public void println(String str)
{
       String name = Thread.currentThread().getName() ;
       PrintWriter file = (PrintWriter) files.get(name);
       if (file == null)
       {
               file = new PrintWriter(new  FileWriter(name)); // you may need to change the name to be a valid filename
               files.put(file);
       }
       
       file.println(str);
       super.println(+ ": " + str); // also printout to the console
}
});
Avatar of narrav
narrav

ASKER

yes, understand now,

so as per you think:

below is the code I am using now should work right??

 for(int index = 0;index < threadCpacsCore.size();index++) {
               ThreadCpacsCore tcp = (ThreadCpacsCore) threadCpacsCore.get(index);

     
               if (null!=ps) {
                                     ps.flush();
                                    try {
                                     ps.close();
                                     } catch (Exception e) {}
                             }


                             try {
                                               fos=new FileOutputStream("Thread"+index+".dat");
                                               bos = new BufferedOutputStream(fos);
                                        ps=new PrintStream(bos);

                               System.setOut(ps);
                               /*
                               {
                             public void println(String str)
                               {
                              super.println(Thread.currentThread().getName() + ": " + str);
                              }
                          });
                          */

            } catch (Exception e) {}




               tcp.start();
          }



          if (null!=ps) {
                    ps.flush();
                   try {
                    ps.close();
           } catch (Exception e) {}

         }
Avatar of narrav
narrav

ASKER

Oh and is it that -- I should wait for process to finish up to see the output in threads STDOUT files???

I am tailing the STDOUT thread files. and

for this, I am testing on 10 threads...

I see only 2 files with only some STDOUT and then it is stopped.. and then rest of files are empty...
Avatar of narrav
narrav

ASKER

well,

I tried with letting process goes to finish as well, still no luck,

I run my code as below:

./runMultiThread.sh 10Threads10.conf 2>stderr.txt&

and I see as below in the end


bash-2.03$ ls -al *dat*
-rw-r--r--   1 rn73264  staff          0 May 11 15:33 Thread0.dat
-rw-r--r--   1 rn73264  staff         27 May 11 15:33 Thread1.dat
-rw-r--r--   1 rn73264  staff          0 May 11 15:33 Thread2.dat
-rw-r--r--   1 rn73264  staff        140 May 11 15:33 Thread3.dat
-rw-r--r--   1 rn73264  staff          0 May 11 15:33 Thread4.dat
-rw-r--r--   1 rn73264  staff          0 May 11 15:33 Thread5.dat
-rw-r--r--   1 rn73264  staff          0 May 11 15:33 Thread6.dat
-rw-r--r--   1 rn73264  staff          0 May 11 15:33 Thread7.dat
-rw-r--r--   1 rn73264  staff          0 May 11 15:33 Thread8.dat
-rw-r--r--   1 rn73264  staff          0 May 11 15:33 Thread9.dat


and I don't get anything in stderr.txt

not sure where stdout is going?
Avatar of narrav
narrav

ASKER

aozarov :

sorry i missed your comment, trying it now
Avatar of TheImmortal
TheImmortal
The problem you are going to run into is that the threads may walk on each other wehn trying to write to the file. Take this following example:

import java.io.*;

public class OTest extends Thread {
 
  FileOutputStream fos=null;
  BufferedOutputStream bos=null;
  PrintStream ps = null;
 
  public static void main(String[] args) {
    OTest o1=new OTest();
    o1.setName("OTest1");
   
    OTest o2=new OTest();
    o2.setName("OTest2");
   
    OTest o3=new OTest();
    o3.setName("OTest3");
   
    OTest o4=new OTest();
    o4.setName("OTest4");
   
    o1.start();
    o2.start();
    o3.start();    
    o4.start();    
  }
 
  public void run() {

    try {
     fos = new FileOutputStream("c:\\" + getName() + ".txt");
     bos = new BufferedOutputStream(fos);
     ps = new PrintStream(bos);
     System.setOut(ps);
    } catch (Exception e) {
      e.printStackTrace();
    }
    while (true) {
      for (int j=0;j<50;j++) {
        for (int x = 0; x < 50000; x++) {}
        writeLog("[" +j+ "] I am Thread " + this.getName());
      }
      break;
     
    }
   
    if (null!=ps) {
      ps.flush();
      ps.close();
    }
  }
 
  private void writeLog(String data) {
   System.out.println(data);
   if (null!=ps) {
     ps.flush();
   }
  }
}


This will result in misplaced log entries. To correct this problem, you could do something like this:


import java.io.*;

public class OTest extends Thread {
 
 
  public static void main(String[] args) {
    OTest o1=new OTest();
    o1.setName("OTest1");
   
    OTest o2=new OTest();
    o2.setName("OTest2");
   
    OTest o3=new OTest();
    o3.setName("OTest3");
   
    OTest o4=new OTest();
    o4.setName("OTest4");
   
    o1.start();
    o2.start();
    o3.start();    
    o4.start();    
  }
 
  public void run() {

    FileOutputStream fos=null;
    BufferedOutputStream bos=null;
    PrintStream ps = null;

    try {
     fos = new FileOutputStream("c:\\" + getName() + ".txt");
     bos = new BufferedOutputStream(fos);
     ps = new PrintStream(bos);
    } catch (Exception e) {
      e.printStackTrace();
    }
    while (true) {
      for (int j=0;j<50;j++) {
        for (int x = 0; x < 50000; x++) {}
        writeLog("[" +j+ "] I am Thread " + this.getName(),ps);
      }
      break;
     
    }
   
    if (null!=ps) {
      ps.flush();
      ps.close();
    }
  }
 
  private static synchronized void writeLog(String data,PrintStream ps) {
   System.setOut(ps);    
   System.out.println(data);
   if (null!=ps) {
     ps.flush();
   }
  }
}

The main difference is the scope of the PrintStream, BufferedOutputStream, and FileOutputStream as well as the synchronization of the writeLog method. This results in all the log entries being placed into the correct location.  The synchronized method should not have a significatn impact on performance, depending on how many System.out.println() statements you have and how often they occur.
Avatar of TheImmortal
TheImmortal
If you implement the writeLog method I have illustrated above, you will be able to tail your log files becuase each output line will be flushed when it is written to STDOUT.  The flush() forces the data to be written.
Avatar of narrav
narrav

ASKER

aozarov:

Its looking lot better with your piece of code, I see:

ls -al Thr*
-rw-r--r--   1 rn73264  staff     131072 May 11 15:46 Thread1
-rw-r--r--   1 rn73264  staff     122880 May 11 15:46 Thread10
-rw-r--r--   1 rn73264  staff     122880 May 11 15:46 Thread2
-rw-r--r--   1 rn73264  staff     131072 May 11 15:46 Thread3
-rw-r--r--   1 rn73264  staff     114688 May 11 15:46 Thread4
-rw-r--r--   1 rn73264  staff     114688 May 11 15:46 Thread5
-rw-r--r--   1 rn73264  staff     114688 May 11 15:46 Thread6
-rw-r--r--   1 rn73264  staff     114688 May 11 15:46 Thread7
-rw-r--r--   1 rn73264  staff     122880 May 11 15:46 Thread8
-rw-r--r--   1 rn73264  staff     122880 May 11 15:46 Thread9


But as you see all filesize are not same..

and I see on console for example:


only last few lines are as below:
Thread7: Finished calculations...
Thread7: ProfileCache hit rate: Infinity%
Thread7: ProductCache hit : 0
Thread7: AccountCache hit: 0
Thread7: The 0 calculations took 10529ms, an average time of Infinityms/calculation


but if I open Thread7 file to see stdout, i don't see these lines.

I guess somethings are being missed to go to these individual files..

any ideas what can be going wrong?

here is the code i am using now:


for(int index = 0;index < threadCpacsCore.size();index++) {
ThreadCpacsCore tcp = (ThreadCpacsCore) threadCpacsCore.get(index);
try{
System.setOut(new java.io.PrintStream(System.out)
{
Map files = new HashMap();

public void println(String str)
{
String name = Thread.currentThread().getName() ;
PrintWriter file = (PrintWriter) files.get(name);
if (file == null)
{
try{
file = new PrintWriter(new  FileWriter(name)); // you may need to change the name to be a valid filename
} catch(IOException e) { e.printStackTrace(); }
files.put(name, file);
}

file.println(str);
super.println(Thread.currentThread().getName() + ": " + str); // also printout to the console
}
});

} catch (Exception e) { e.printStackTrace();}




tcp.start();
}
Avatar of narrav
narrav

ASKER

TheImmortal,

In your 2nd example of code (with synchronization), I see to do a System.setOut(ps) in writeLog..

well, do you mean to do it this way??

not sure, but if i use your way, eveything is going under Thread9.dat file for all the threads
Avatar of narrav
narrav

ASKER

below is what i am doing..

I have 2 classes
MultiThreadCpacsCoreTest

main method:
fileName = args[0];
       System.out.println("FileName is: " + fileName);
       MultiThreadCpacsCoreTest m = new MultiThreadCpacsCoreTest();
       m.executeFile();
      }

executeFile method:

         for(int index = 0;index < threadCpacsCore.size();index++) {
               ThreadCpacsCore tcp = (ThreadCpacsCore) threadCpacsCore.get(index);
                tcp.start();
   }

and ThreadCpacsCore

in run i have done:

public void run() {

             FileOutputStream fos=null;
           BufferedOutputStream bos=null;
         PrintStream ps = null;


         try {

                    fos = new FileOutputStream(getName() + ".dat");
                    bos = new BufferedOutputStream(fos);
                  ps = new PrintStream(bos);

                  writeLog("I am Thread " + this.getName(),ps);

            //System.out.println("Executing Thread : " + this.getName());

            TestDriver testDriver = new TestDriver(inputFile, outputFile, runOption,-1);
            testDriver.testCalculation();

            if (null!=ps) {
                  ps.flush();
                  ps.close();
                }



} catch(Exception e) {
System.out.println("Got Exception  " + e);
      }
}

in this ThreadCpacsCore  class, I have declared method writeLog as you described
Avatar of aozarov
aozarov
You don't need to put it inside a loop (put it as the first line in your main method [as is] and it should work for you fine - it did for me).

System.setOut(new java.io.PrintStream(System.out)
                        {
                              Map files = new HashMap();

                              public void println(String str)
                              {
                                    String name = Thread.currentThread().getName() ;
                                    PrintWriter file = (PrintWriter) files.get(name);
                                    if (file == null)
                                    {
                                          try
                                          {
                                                file = new PrintWriter(new FileWriter(name)); // you may need to change the name to be a valid filename
                                                files.put(name, file);
                                          }
                                          catch(IOException e) { e.printStackTrace(); }
                                    }

                                    if (file != null)
                                    {
                                          file.println(str);
                                          file.flush();
                                    }
                                    super.println(name + ": " + str); // also printout to the console
                              }
                        });
Avatar of TheImmortal
TheImmortal
Copy my code out of the post (the code sample with the synchronized method), place it in a file and compile it. Give it a run and see the results.  You will see that each thread writes its messages to the appropriate file.
Avatar of narrav
narrav

ASKER

aozarov :

looks like it is working for me.. sorry, i thought i have to put in a loop because Thread name only comes up in executeFile function .. where executeFile reads a file to see what is the ThreadName and other params, and so I put it in executeFile function in a loop while looping through the array list of Threads..

not sure how is it working -- i will have to understand it more .. but looks like it is working..
how can i make sure that Thread9 file has contents only relevant to Thread9 and not relevant to Thread7 .. any easy way to do it??

I see  on console STDOUT - I see ThreadName and then the output.. I am not getting the same thing in Thread's stdout file..
I just see the STDOUT -- without the threadname in Thread's stdout file..



TheImmortal,

your code is running fine and perfect, but for some reason, I am uable to integrate in my code ..i am so sorry, i have limited knowledge, so trying to understand it


Avatar of TheImmortal
TheImmortal
Basically, my code just has loops that simulate a busy thread. It is mainly just to illustrate how to override the STDOUT per thread. The loops are nothing to pay attention to for your code.
Avatar of aozarov
aozarov
>>how can i make sure that Thread9 file has contents only relevant to Thread9 and not relevant to Thread7 .. any easy way to do it??
The code already take care of that as it uses the FileWriter of the calling thread (see Thread.currentThread().getName() ;)

I see  on console STDOUT - I see ThreadName and then the output.. I am not getting the same thing in Thread's stdout file..
I just see the STDOUT -- without the threadname in Thread's stdout file..
I thought that is not needed there as all the output of that file related a specific thread.
You can change
file.println(str);
to
file.println(name + ": " + str);
if you want to include the thread name inside the thread file (and you will see that file Thread9 contains only content with prefix "Thread9 ")
Avatar of narrav
narrav

ASKER

okay,
aozarov your solution is working.
TheImmortal : I still have to test and integrate your code. i am sure thats another solution. i just have to test if it works in my application.

on the original question which i am almost near to finish up,

after doing deep cloning, i pass the clonedProfile object to filterIneligible charges right?
so I modify this object  -- clonedProfile.. after that before returning this clonedProfile from find method, if i do
profile = clonedProfile, will it not modify the cache object.. so I don't have to remove the object from cache and put it back..

because profile still points to object in cache right??

also, i was having the same problem (ConcurrentModification exception) while doing a DB.cleanup(Connection) in my abstractFind finally method ..

DB.cleanup(Connection conn) looks as below:

public static void cleanUp(Connection conn) throws SQLException
{      if (conn != null)
{      IObjectPool pool = null;
IPooledObject pooledConnection = null;
for (Iterator i = pools.values().iterator(); i.hasNext();)
{      pool = (IObjectPool)i.next();

pooledConnection = pool.findPooledObjectByObject(conn);
if (pooledConnection != null)
{      pooledConnection.release();
// found the right connection, so get out of here!
break;
}

}
}
}

where pools is a static class variable --
Map pools = new HashMap()

if i do a synchronized(pool) in a loop:

as below:

synchronized(pool) {
                        pooledConnection = pool.findPooledObjectByObject(conn);
                        if (pooledConnection != null)
                        {      pooledConnection.release();
                              // found the right connection, so get out of here!
                              break;
                        }
                    }

it works fine..

is this a good way to it??

ofcourse synchronizing will again impact performance.. any better way to deal with it..??


lastly, i was looking into link argoUML statediagram --  as i understand, i have to draw state diagram for each class and create process flow based on this .. i was thinking if there is a easy way like importing the whole project and it creates a process flow diagram for other ddevelopers or new team members to figure out to see everything.
well, i didn't look completely in argoUML, may be it has some cool features which i need to understand .. but with first look, i could not find very easy way if there is to create such a diagram..

i don't know if rational rose is a better tool or not.. i understand it is not free, looks like we already have a licence for it.. but not sure if i can use and if it would make creating this diagram easy for me.. any ideas???
i have eclipse here.. so if u think any plugin in eclipse can do this job .. let me know where can i get this plugin from please.

sorry to bother you with so many questions.. but at this place, i am getting the best advise, and best support, so just getting curious to understand and learn as much as possible from u guys
thanks and regards

Avatar of TheImmortal
TheImmortal
While I understand your desire, and need, to keep this thread going with new questions. It eventually must come to an end and give birth to a new thread. Please give this some consideration.
Avatar of aozarov
aozarov
>> profile = clonedProfile, will it not modify the cache object.. so I don't have to remove the object from cache and put it back..
>> because profile still points to object in cache right??
After you assigne to profile teh cloned profile it is not pointing to the same object the cache is pointing but rather to its clone.
If you wan to update your cache with it (which again I can't understand why you would do such a thing) then you will need to put profile back to the cache.

>> ofcourse synchronizing will again impact performance.. any better way to deal with it..??
That should be fine as the finder will be called only for items which are not in the cache (and then the query will have much bigger impact then this operation).

>> was thinking if there is a easy way like importing the whole project
ArgoUML can import java files.
Avatar of narrav
narrav

ASKER

Ok buddies,

I will close this question now.. this thread is very helpful to me. i will open up a new thread
with other questions and I really appreciate if you guys reply since u alreayd know alot about my project and its technical stuff

you all have helped me alot to make me understand the things and i really appreciate it

i will split the points and close this question.

thanks alot
Avatar of narrav
narrav

ASKER

aozorov:

just one last questtion since i am little confused with this:
you said:

>>After you assigne to profile teh cloned profile it is not pointing to the same object the cache is pointing but rather to its clone.
If you wan to update your cache with it (which again I can't understand why you would do such a thing) then you will need to put profile back to the cache.


but where am i assigning to profile the clonedprofile.
i created a new object -- clonedProfile
and i am doing

clonedProfile = (ChargeProfile)profile.clone();

so profile is one object, cloned profiled is another object.. and both are indepandant right??
Avatar of aozarov
aozarov
>> so profile is one object, cloned profiled is another object.. and both are indepandant right??
Right.

I was refering to tihs: "profile = clonedProfile"