Project

General

Profile

Bug #7643

memory leak in AssociatedThread

Added by Constantin Asofiei almost 3 years ago. Updated almost 3 years ago.

Status:
Test
Priority:
Normal
Assignee:
Constantin Asofiei
Target version:
-
Start date:
Due date:
% Done:

100%

billable:
No
vendor_id:
GCD
case_num:
version_reported:
version_resolved:
reviewer:
production:
No
env_name:
topics:

History

#1 Updated by Constantin Asofiei almost 3 years ago

In a large GUI application, in a thread dump SecurityManager.threadIds is reported to have a size of 120k (for reference, this is from the thread heap dump from #7490).

The problem is the AssociatedThread code. The context is assigned via (for i.e. a PSTimer thread):

SecurityManager.assignContext(SecurityContext) line: 8427    
SecurityManager.setInitialSecurityContextWorker(boolean, SecurityContextStack) line: 5001    
SecurityManager.forceSecurityContext(SecurityContextStack, boolean) line: 4823    
AssociatedThread(ContextAwareThread).run() line: 130    
AssociatedThread.run() line: 79

and the popAllSecurityContext code does not call dropInitialSecurityContext - instead it has this code:
      // exit the initial context
      endContext(cts, ctx);

We are missing something here. We can't just call dropInitialSecurityContext after endContext. We need another API I think which is a combination of both.

#3 Updated by Constantin Asofiei almost 3 years ago

  • Status changed from New to Review
  • Assignee set to Constantin Asofiei
  • % Done changed from 0 to 100

Created task branch 7643a from trunk rev 14675.

The fix is in 7643a rev 14676.

#4 Updated by Greg Shah almost 3 years ago

Code Review Task Branch 7643a Revision 14676

Shouldn't we just change endContext() to remove the thread id as part of its cleanup?

#5 Updated by Constantin Asofiei almost 3 years ago

Greg Shah wrote:

Code Review Task Branch 7643a Revision 14676

Shouldn't we just change endContext() to remove the thread id as part of its cleanup?

Actually, I don't think we really need threadIDs map - we can replace this with a public static ThreadLocal<Integer> var.

#6 Updated by Greg Shah almost 3 years ago

Constantin Asofiei wrote:

Greg Shah wrote:

Code Review Task Branch 7643a Revision 14676

Shouldn't we just change endContext() to remove the thread id as part of its cleanup?

Actually, I don't think we really need threadIDs map - we can replace this with a public static ThreadLocal<Integer> var.

I like it.

#7 Updated by Constantin Asofiei almost 3 years ago

I'm thinking of keeping this new ThreadLocal field set only once; previously, for a thread, the value got changed each time a new security context got assigned. This may create confusion in the logs - as there we already have the FWD Session ID, and the Thread ID just means what Java Thread logged this message.

#8 Updated by Greg Shah almost 3 years ago

Agreed.

#9 Updated by Constantin Asofiei almost 3 years ago

Fixed in 7643a rev 14677.

#10 Updated by Greg Shah almost 3 years ago

Code Review Task Branch 7643a Revision 14677

It looks good.

You can merge to trunk.

#11 Updated by Constantin Asofiei almost 3 years ago

Branch 7643a was merged to trunk rev 14677 and archived.

#12 Updated by Greg Shah almost 3 years ago

  • Status changed from Review to Test

Also available in: Atom PDF