FWD » Core Development » Base Language

The problem is that the following code can be executed in both PSTimer threads, so, the context for ServerEvent.events is created twice, the first time for the first server event instance, and second time for the second, and the registration of the first server event get lost.

    ServerEvent$1.initialValue() line: 116    
    ServerEvent$1.initialValue() line: 1    
    ServerEvent$1(ContextLocal<T>).getImpl(boolean) line: 542    
    ServerEvent$1(ContextLocal<T>).get() line: 454    
    ServerEvent.<init>(long, String, Runnable, boolean) line: 189    
    ServerEvent.<init>(long, String, Runnable) line: 162    
    ComServer.emit(ComEvent, boolean) line: 204    
    PSTimer.fireEvent() line: 678    
    PSTimer.run() line: 633    
    AssociatedThread(ContextAwareThread).run() line: 136    
    AssociatedThread.run() line: 1    

Code Review Task Branch 8393b Revision 15373

I don't think it is correct to synchronize on dependencies. That is arbitrary and inconsistent with the other synchronization in use. Should it be synchronized on contextStorage?

Greg Shah wrote:

Code Review Task Branch 8393b Revision 15373

I don't think it is correct to synchronize on dependencies. That is arbitrary and inconsistent with the other synchronization in use. Should it be synchronized on contextStorage?

Indeed. we can use any object singleton for synchronization. Agreed, the dependencies object causes questions, since it is otherwise unrelated to the purpose. The only problem with dependencies is if we ever want to change this variable definition to something mutable, we will also need to modify its usage in the synchronization.

We cannot use contextStorage for this, since this variable is used at the client side only, and is always null at the server side.

To make code more readable, we can add one more variable specially for the synchronization:

private final static Object lock = new Object();

Yes, that makes sense. Should we switch all of the current synchronization to that same lock object?

Greg Shah wrote:

Yes, that makes sense. Should we switch all of the current synchronization to that same lock object?

Yes, we can use this special lock object for all "static" synchronization places in ContextLocal, except for the cases when the contextStorage is used to synchronize the access to contextStorage only, which is perfectly OK.

Also, I think, the following code should by executed synchronized:

         if (contextStorage == null)
         {
            contextStorage = new HashMap();
         }

Also, I think, the following code should by executed synchronized:

Agreed.

Vladimir Tsichevski wrote:

Greg Shah wrote:

Yes, that makes sense. Should we switch all of the current synchronization to that same lock object?

Yes, we can use this special lock object for all "static" synchronization places in ContextLocal, except for the cases when the contextStorage is used to synchronize the access to contextStorage only, which is perfectly OK.

The access to contextStorage is intermixed with the access to other synchronized data, so you are right, to be sure, we better use one lock object for all statically synchronized sections.

Vladimir Tsichevski wrote:

I retested the issue once again: it appears sometimes still (but rarely), despite the problem with ServerEvent initialization registry is now fixed, and the registry is not overwritten.

The problem is that the whole branch must be executed synchronized:

      else if (mode == MODE_SERVER)
      {
         ...
      }

Otherwise the following call can be executed more than one time per client:

    at com.goldencode.p2j.ui.client.event.ServerEvent$1.initialValue(ServerEvent.java:117)
    at com.goldencode.p2j.ui.client.event.ServerEvent$1.initialValue(ServerEvent.java:1)
    at com.goldencode.p2j.security.ContextLocal.getImpl(ContextLocal.java:545)
    at com.goldencode.p2j.security.ContextLocal.get(ContextLocal.java:455)
    at com.goldencode.p2j.ui.client.event.ServerEvent.<init>(ServerEvent.java:192)
    at com.goldencode.p2j.ui.client.event.ServerEvent.<init>(ServerEvent.java:163)
    at com.goldencode.p2j.comauto.ComServer.emit(ComServer.java:204)
    at com.goldencode.p2j.extension.PSTimer.fireEvent(PSTimer.java:678)
    at com.goldencode.p2j.extension.PSTimer.run(PSTimer.java:633)
    at com.goldencode.p2j.security.ContextAwareThread.run(ContextAwareThread.java:136)
    at com.goldencode.p2j.security.AssociatedThread.run(AssociatedThread.java:1)

I checked this by setting a conditional breakpoint in initValue, which dumps current thread stack trace.

Code Review Task Branch 8393b Revision 15374

In checkCaller(), the final int prefixLength = "com.goldencode.p2j.".length(); should be a static final data member so that it is not recalculated multiple times.

Otherwise the change is fine. It needs heavy regression testing to ensure that there are no deadlocks. ChUI regression testing and unit testing with multiple applications will be needed.

Constantin: Please review.

Greg Shah wrote:

Code Review Task Branch 8393b Revision 15374

In checkCaller(), the final int prefixLength = "com.goldencode.p2j.".length(); should be a static final data member so that it is not recalculated multiple times.

Done. After this fixed and the branch rebased, the new revno is 15389.

Constantin: Please review.

What branch exactly? There is no 8398b.

Constantin Asofiei wrote:

What branch exactly? There is no 8398b.

The branch is 8393b, sorry.

The fixes for the synchronization of contextStorage look OK. But, for getImpl, the synchronization in:

      else if (mode == MODE_SERVER)
      {
         synchronized (lock)
         {
            // use the optimized get from ThreadLocal if it exists

Code Review 8393b Revision 15375

Minor issue: FWD_CLASSES_PREFIX_LENGHT should be FWD_CLASSES_PREFIX_LENGTH.

Of course, if 9110a resolves this then we will drop 8393b.

After this fixed and the branch rebased, the new revno is 15389.

BTW, the branch I see is not rebased.

Greg Shah wrote:

After this fixed and the branch rebased, the new revno is 15389.

BTW, the branch I see is not rebased.

I cannot do this due to the problem with trunk :-(. I complained on this already in #7143.

Vladimir Tsichevski wrote:

Greg Shah wrote:

After this fixed and the branch rebased, the new revno is 15389.

BTW, the branch I see is not rebased.

I cannot do this due to the problem with trunk :-(. I complained on this already in #7143.

Use devsrv01 - checkout both trunk and 8393b, unbind, rebase and push.

I found another problem, which currently causes deadlock in a customer app with 8393b.

The problem is in TemporaryDatabaseManager, when the perUserTempDatabases.PrivateTempDbManager is accessed for modification:

The addition code is:

         synchronized (perUserTempDatabases)
         {
            String tempDbName = TemporaryDatabaseManager.getMyTempDbName();
            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ problem here
            if (!perUserTempDatabases.contains(tempDbName))
            {
               ...               
               perUserTempDatabases.add(tempDbName);
            }

And removal:

         String tempDbName = getMyTempDbName();
         synchronized (perUserTempDatabases)
         {
            perUserTempDatabases.remove(tempDbName);
            ...
         }

The removal code is OK: the call to getMyTempDbName() is out of synchronization block, while the addition code causes deadlock:
the call to getMyTempDbName() is inside the synchronization block, in it uses thread-context access, which causes dead-lock with the following two threads:

Daemon Thread [Conversation [0000000B:AppserverProcess]] (Suspended)    
    owns: HashSet<E>  (id=12385)    
    waiting for: Object  (id=12386)    
    TemporaryDatabaseManager$PrivateTempDbManager$1(ContextLocal<T>).getImpl(boolean) line: 529    
    TemporaryDatabaseManager$PrivateTempDbManager$1(ContextLocal<T>).get() line: 467    
    TemporaryDatabaseManager$PrivateTempDbManager.getMyTempDbName() line: 355    
    TemporaryDatabaseManager.getMyTempDbName() line: 233    
    TemporaryDatabaseManager.access$300() line: 95    
    TemporaryDatabaseManager$PrivateTempDbManager.initializeMyTempDatabase() line: 311    
    TemporaryDatabaseManager.initializeMyTempDatabase() line: 183

Daemon Thread [Conversation [0000000A:Process]] (Suspended)    
    owns: Object  (id=12386)    
    waiting for: HashSet<E>  (id=12385)    
    TemporaryDatabaseManager$PrivateTempDbManager.initializeMyTempDatabase() line: 311    
    TemporaryDatabaseManager.initializeMyTempDatabase() line: 183    
    PersistenceFactory.getInstance(Database, DatabaseConfig) line: 208    
    PersistenceFactory.getInstance(Database) line: 147    
    TemporaryBuffer(RecordBuffer).initialize(boolean) line: 7192    
    TemporaryBuffer.initialize(boolean) line: 5992    

Note: this does not mean 8393b is innocent, it is just another source of potential problems.

Vladimir Tsichevski wrote:

Constantin Asofiei wrote:

The fixes for the synchronization of contextStorage look OK. But, for getImpl, the synchronization in:
[...]
I don't think is OK

In a sense, I agree, this currently causes deadlock in multiple FWD threads. But at least in two places this is because non-deadlock-safe code, which can be (should be?) fixed.

please see the 9110a changes, and if these solve the issue you have in this task.

I re-tested with the latest (rev. 15415) trunk with the changes from 9110a merged, and the #9037 issue still remains.

Now I will try to merge the 9110a changes with 8393b and re-test.

This did not solve the problem either, we probably still need more locking in ContextLocal and changes in other classes to avoid resulting deadlocks.

• use a ReentrantReadWriteLock saved in the SecurityContext instance (Greg, IIRC this is a singleton for each FWD context, right?)
• in each non-static ContextLocal method, resolve the ReentrantReadWriteLock from the SecurityContext, and:
  • if the method is read-only, then obtain a read lock and lock on that
  • if the method writes, then obtain a write lock and lock on that
• synchronization on the static ContextLocal fields can be removed completely:
  • contextStorage can be made a synchronized (identity) map
  • dependencies can be made a synchronized TreeMap.

This way I think we can remove the class-level synchronization completely, and I hope it will also show some performance improvement, as the locks will be more granular.

Regarding the changes in 9037a, we still have private synchronized void setMode(), which doesn't seem right; and even as they are, the changes are still built on the previous concept of synchronizing the access to ContextLocal.

Constantin Asofiei wrote:

Regarding the changes in 9037a...

Branch 9037a never existed :-(. The change is in 8393b, freshly rebased :-)

Vladimir Tsichevski wrote:

Constantin Asofiei wrote:

Regarding the changes in 9037a...

Branch 9037a never existed :-(. The change is in 8393b, freshly rebased :-)

Sorry, yes, I meant 8393b.

SecurityContext instance (Greg, IIRC this is a singleton for each FWD context, right?)

In most cases, yes it is indeed a dedicated instance per context/session.

I think there is a weird exception for the SecurityCache.initContext (associated with the "system session") which is used as the initial context for any non-unique (i.e. the setInitialSecurityContextWorker() first parameter is false) session processed by SecurityManager.setInitialSecurityContextWorker().

I'm not sure how much we depend on that but I have a sneaking feeling it is a bit of badness waiting to bite us. If that is resolved without regressions, then your plan will work nicely.

Review:
Not sure if locking in ContextLocal would introduce even more problems, but I've found that out while working on #9243 where I tried to introduce ReentrantReadWriteLock into ContextLocal.getImpl for MODE_UNSET - I failed so far.
In #9243 the problem was that objects are creating other objects on demand when they are needed and that happens on different threads, and I think here we have similar situation. I am not sure if Constantin's solution would work due to complex dependencies between ContextLocal stored objects - I guess it still would lock if two threads would request write lock.

As an alternative solution I was considering three scenarios:
- pre-initialization - create some default ContextLocal objects on session start - that would require identifying objects that are needed for given session type
- delayed initialization - create objects on request but initialize them when first accessed - that would require some wrappers and not sure if it would prevent deadlocks
- disable object initialization by default in ContextLocal.get and let upstream code handle it, that would require a lot of change of FWD code.

Tomasz Domin wrote:

As an alternative solution I was considering three scenarios:
- pre-initialization - create some default ContextLocal objects on session start - that would require identifying objects that are needed for given session type

+1. I believe all objects should be created at session startup in a predetermined order to eliminate the possibility of encountering race conditions.

Note: the fix for #10206 merged as rev. 16156 has not resolved this issue.

Vladimir, what is the status of this issue? It is marked as 100% done, should it be reviewed?

Hynek Cihlar wrote:

Vladimir, what is the status of this issue? It is marked as 100% done, should it be reviewed?

The 8393b fixes the original problem, but there is a general problem with ContextLocal. IMO most reliable solution is proposed in #9037-42.