[jira] [Commented] (DIRMINA-1110) Ordered Executors concurrency

["Emmanuel Lecharny (JIRA)" <ji...@apache.org>]

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

Emmanuel Lecharny commented on DIRMINA-1110:
--------------------------------------------

Patch applied in 2.1.X branch.

Many thanks Guus !

(I think it's going to fix DIRMINA-1113)

> Ordered Executors concurrency
> -----------------------------
>
>                 Key: DIRMINA-1110
>                 URL: https://issues.apache.org/jira/browse/DIRMINA-1110
>             Project: MINA
>          Issue Type: Improvement
>    Affects Versions: 2.1.2
>            Reporter: Guus der Kinderen
>            Priority: Major
>         Attachments: mina-ordered-executors.patch
>
>
> MINA contains two ThreadPoolExecutor implementations that maintains the order of IoEvents per session:
> * OrderedThreadPoolExecutor
> * PriorityThreadPoolExecutor
> This issue applies to both.
> A private class called {{SessionTasksQueue}} (confusingly using different names in both implementations, which is an undesired side-effect having code duplication) is used to represent the ordered collection of events to be processed for each individual session. It also contains a boolean that represents the state of the queue prior to addition of the task: 'true' if the collection was empty ("processing complete"), otherwise 'false'. This queue is stored as an attribute on the session.
> An {{IoEvent}} is _scheduled_ for execution by being passed to the {{execute}} method. "Scheduling" an {{IoEvent}} involves identifying the session that it belongs to, and placing it in its SessionTaskQueue. Finally, the session itself is, conditionally (more in this later), placed in a queue named {{waitingSessions}}.
> The {{IoEvent}} execution itself is implemented by {{Runnable}} implementations called {{Worker}}. These workers take their workload from the {{waitingSessions}} queue, doing blocking polls.
> The placing of the Session on the {{waitingSessions}} queue depends on the state of the {{SessionTasksQueue}}. If it was empty ("processingComplete"), the session is placed on the {{waitingSessions}} queue. There is comment that describes this as follows:
> {quote}As the tasksQueue was empty, the task has been executed immediately, so we can move the session to the queue of sessions waiting for completion.{quote}
> As an aside: I believe this comment to be misleading: there's no guarantee that the task has actually been executed immediately, as workers might still be working on other threads. On top of that, as both the production on, and consumption of that queue is guarded by the same mutex, I think it's quite impossible that the task has already been executed. A more proper comment would be:
> {quote}Processing of the tasks queue of this session is currently not scheduled or underway. As new tasks have now been added, the session needs to be offered for processing.{quote}
> The determination if the session needs to be offered up for processing is based on an evaluation of the state of the {{sessionTasksQueue}} that happens under a mutex. The actual offer of the session for processing (adding the session to the {{waitingSessions}} queue, is not. We believe, but have not been able to conclusively prove, that this can lead to concurrency issues, where a session might not be queued, even though it has tasks to be executed. Nonetheless, we suggest moving the addition of the session to  {{waitingSessions}} into the guarded code block. At the very least, this reduces code complexity.
> The patch attached to this issue applies this change. It also changes the name of the Session tasks queue in {{PriorityThreadPoolExecutor}}, to match the name used in {{OrderedThreadPoolExecutor}}



--
This message was sent by Atlassian JIRA
(v7.6.3#76005)