[ https://issues.apache.org/jira/browse/DIRMINA-1110?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17143013#comment-17143013 ]
Jonathan Valliere commented on DIRMINA-1110: -------------------------------------------- For the record, the above patch was reverted at 24fc810141081119273a71f61b08c94aeaf43d5c by my request because I'm not sure the patch actually fixed anything. > 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 (v8.3.4#803005) --------------------------------------------------------------------- To unsubscribe, e-mail: dev-unsubscr...@mina.apache.org For additional commands, e-mail: dev-h...@mina.apache.org