Guus der Kinderen created DIRMINA-1110:
------------------------------------------
Summary: 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
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)
