Mailing-List: contact dev-help@shiro.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@shiro.apache.org
Date: Tue, 28 Jun 2016 16:49:57 +0000 (UTC)
From: "ASF GitHub Bot (JIRA)" <jira@apache.org>
To: dev@shiro.apache.org
Message-ID: <JIRA.12735405.1408542689000.8702.1467132597062@Atlassian.JIRA>
In-Reply-To: <JIRA.12735405.1408542689000@Atlassian.JIRA>
References: <JIRA.12735405.1408542689000@Atlassian.JIRA> <JIRA.12735405.1408542689722@arcas>
Subject: [jira] [Commented] (SHIRO-515)
 ExecutorServiceSessionValidationScheduler leaks resources due to improper
 synchronization
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 7bit
archived-at: Tue, 28 Jun 2016 16:49:58 -0000


    [ https://issues.apache.org/jira/browse/SHIRO-515?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15353361#comment-15353361 ] 

ASF GitHub Bot commented on SHIRO-515:
--------------------------------------

GitHub user ankon opened a pull request:

    https://github.com/apache/shiro/pull/21

    SHIRO-515: Better synchronization for the session validation

    
You can merge this pull request into a Git repository by running:

    $ git pull https://github.com/Collaborne/shiro issue/SHIRO-515

Alternatively you can review and apply these changes as the patch at:

    https://github.com/apache/shiro/pull/21.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

    This closes #21
    
----
commit d0fa29360dca827c5e293ce112b6c2d646bb6124
Author: Andreas Kohn <andreas.kohn@gmail.com>
Date:   2016-06-28T16:46:51Z

    SHIRO-515 [1/2] Mark the ExecutorServiceSessionValidationScheduler "enabled" even with a 0 interval

commit dfb0e3691e3bddd6f0c3f92116d21e4039ee5cab
Author: Andreas Kohn <andreas.kohn@gmail.com>
Date:   2016-06-28T16:47:18Z

    SHIRO-515 [2/2] 'synchronized' #disableSessionValidation()

----


> ExecutorServiceSessionValidationScheduler leaks resources due to improper synchronization
> -----------------------------------------------------------------------------------------
>
>                 Key: SHIRO-515
>                 URL: https://issues.apache.org/jira/browse/SHIRO-515
>             Project: Shiro
>          Issue Type: Bug
>          Components: Session Management
>    Affects Versions: 1.2.3
>            Reporter: Andreas Kohn
>            Priority: Critical
>              Labels: patch
>         Attachments: SHIRO-515-race.patch
>
>
> 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
(v6.3.4#6332)