mina-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Guus der Kinderen (JIRA)" <j...@apache.org>
Subject [jira] [Created] (DIRMINA-1110) Ordered Executors concurrency
Date Wed, 15 May 2019 14:23:00 GMT
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

View raw message