[
https://issues.apache.org/jira/browse/DIRMINA-1110?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16847966#comment-16847966
]
Jonathan Valliere edited comment on DIRMINA-1110 at 5/24/19 11:45 PM:
----------------------------------------------------------------------
The producer and consumer fall under the same mutex. The consumer sets
{{processingCompleted}} to {{true}} when empty. The producer sets
{{processingCompleted}} to {{false}} when an item is added. These two actions
are correct and concurrent. There is no way I can see that the problem you are
trying to solve will actually occur. Given the way it is designed
{{processingCompleted}} isn't even necessary as {{queue.size() == 0}} would
work. The {{tasksQueue}} doesn't even need to be a concurrent structure as
implemented.
was (Author: johnnyv):
The producer and consumer fall under the same mutex. The consumer sets
{{processingCompleted}} to {{true}} when empty. The producer sets
{{processingCompleted}} to {{false}} when an item is added. These two actions
are correct and concurrent. There is no way I can see that the problem you are
trying to solve will actually occur. Given the way it is designed
{{processingCompleted}} isn't even necessary as {{queue.size() == 0}} would
work.
> 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)
