shiro-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Andreas Kohn (JIRA)" <>
Subject [jira] [Created] (SHIRO-515) ExecutorServiceSessionValidationScheduler instances leaking due to improper synchronization
Date Wed, 20 Aug 2014 13:52:27 GMT
Andreas Kohn created SHIRO-515:

             Summary: ExecutorServiceSessionValidationScheduler instances leaking due to improper
                 Key: SHIRO-515
             Project: Shiro
          Issue Type: Bug
          Components: Session Management
    Affects Versions: 1.2.3
            Reporter: Andreas Kohn
            Priority: Critical

Related to SHIRO-514, which talks about a bunch of threads with "ugly" names. This ticket
is about _why_ there are multiple of those in the first place.

>From my heap dump I can see about 71 ThreadFactory instances of ExecutorServiceSessionValidationScheduler$1,
each actually linking to a distinct ScheduledThreadPoolExecutor, and each running one thread.
However, there are only 2 ExecutorServiceSessionValidationScheduler instances.

The problem is in AbstractValidatingSessionManager#enableSessionValidationIfNecessary(): if
this method is called from multiple threads, and those threads race in such a way that multiple
ones see an unset/not-yet-enabled scheduler, they will each run #enableSessionValidation(),
which creates a scheduler, sets it (still disabled), and then runs SessionValidationScheduler#enableSessionValidation(),
which might set its 'enabled' flag to true. In the case of the default ExecutorServiceSessionValidationScheduler
this creates the thread pool executor, and schedules the validation run iff there is a positive
interval configured.

The race is not particularly unlikely when an application using shiro is deployed and many
requests hit it immediately. As Shiro is leaking both JVM and system resources here it's also
not a benign race. 

The attached patch changes the sessionValidationScheduler to be volatile, and makes #enableSessionValidation()
as  well as #disableSessionValidation()  {{synchronized}}.
#enableSessionValidationIfNecessary() is not synchronized in the common case to avoid too
much blocking when everything is properly warmed up, but rather uses double-checked locking.
Shiro targets a 1.5 JVM from what I can see, so this should be ok.

Note that callers can possibly still produce issues by modifying the field directly.

Additionally the ExecutorServiceSessionValidationScheduler was changed to report 'enabled'
when someone tried enabling the validation, regardless of whether the interval was 0 or not.
The rationale here is that someone might try disabling the validation by setting the interval,
which would mean useless calls to #enableSessionValidation() in every request.

This message was sent by Atlassian JIRA

View raw message