[jira] [Comment Edited] (DIRMINA-738) Using IoEventQueueThrottler with a WriteRequestFilter can lead to hangs

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

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

Emmanuel Lecharny edited comment on DIRMINA-738 at 9/5/14 11:02 AM:
--------------------------------------------------------------------

The IoEventQueueThrottle has two some serious problems in its current implementation/application.

1. The internal counter variable is not working correctly.
2. The notification of waiting threads (e.g. from an ExecutorFilter) is deceptive. 

Besides this, the implementation is quite useful if you want to have a server running with a limited memory footprint. The threshold counter needs not to be decreased (or increased) during operation. It fulfils the specification for the maximum amount of memory to use (for the transferred data in total over all connections).


To the identified problems:

1. The incorrect behaviour of the counter variable is not an implementation issue of the IoEventQueueThrottle but a bad 'contract' in its application.

It is implicitly assumed that any IoEvent offered(...) and polled(...) will always have the same size. But this is not true. It seems to me that if an IoBuffer is written immediately before closing the session it's size can change between the calls to offered() and polled(). During my observation the polled
size decreased which lead to an increasing counter approximating the threshold.  

Probably there reason is a (sometimes) missing flip() somewhere else to the IoBuffer but this issue could also be resolved from the IoEventQueueThrottle by storing the estimated size between the calls to offered() and polled(). BTW the order of the calls to these functions is not sure to be in this order. An IoEvent can be polled before being offered. So I propose the following changes: 

1.1. Add a hash map for the event sizes:

{code}
    private final ConcurrentMap<Object,Integer> eventSizes = new ConcurrentHashMap<Object,Integer>();
{code}

1.2. Add a function to maintain the correct event sizes assuming that this function will be called twice for every IoEvent (that's the contract for the implementation):

{code}
    private int maintainSize(IoEvent event) {
        int eventSize = estimateSize(event);
        Integer stored = eventSizes.putIfAbsent(event, new Integer(eventSize));
        if (stored != null) {
                int storedSize = stored.intValue();
                if (eventSize != storedSize) {
                        LOGGER.debug(Thread.currentThread().getName() + " size of IoEvent changed from " + storedSize + " to " + eventSize);
                }
                eventSize = storedSize;
                eventSizes.remove(event);
        }
        return eventSize;
    }
{code}

1.3. Replace the call to estimateSize() inside offered() and polled() with a call to maintainSize()

If the state is logged and all sessions are somehow idle you could now see that the counter will return to it's initial value 0.



2. If the internal counter reaches or crosses the threshold *all* NioProcessor threads offering IoEvents will be stuck waiting for the lock. But when the counter drops below the threshold only *one* waiting thread will be notified.

Assume the session of the *one* continuing NioProcessor thread would have no more data transfer all then other threads would keep on waiting (NB: if you had specified an idle time for your server IoEvents could have notified all other waiting threads after a while hiding this problem).    

So I propose the following change: 

The unblock() method should call notifyAll() instead of notify().


Both problems together could mutually amplify to block forever.

I would appreciate any comments.


was (Author: wimmer):
The IoEventQueueThrottle has two some serious problems in its current implementation/application.

1. The internal counter variable is not working correctly.
2. The notification of waiting threads (e.g. from an ExecutorFilter) is deceptive. 

Besides this, the implementation is quite useful if you want to have a server running with a limited memory footprint. The threshold counter needs not to be decreased (or increased) during operation. It fulfils the specification for the maximum amount of memory to use (for the transferred data in total over all connections).


To the identified problems:

1. The incorrect behaviour of the counter variable is not an implementation issue of the IoEventQueueThrottle but a bad 'contract' in its application.

It is implicitly assumed that any IoEvent offered(...) and polled(...) will always have the same size. But this is not true. It seems to me that if an IoBuffer is written immediately before closing the session it's size can change between the calls to offered() and polled(). During my observation the polled
size decreased which lead to an increasing counter approximating the threshold.  

Probably there reason is a (sometimes) missing flip() somewhere else to the IoBuffer but this issue could also be resolved from the IoEventQueueThrottle by storing the estimated size between the calls to offered() and polled(). BTW the order of the calls to these functions is not sure to be in this order. An IoEvent can be polled before being offered. So I propose the following changes: 

1.1. Add a hash map for the event sizes:

    private final ConcurrentMap<Object,Integer> eventSizes = new ConcurrentHashMap<Object,Integer>();

1.2. Add a function to maintain the correct event sizes assuming that this function will be called twice for every IoEvent (that's the contract for the implementation):

    private int maintainSize(IoEvent event) {
        int eventSize = estimateSize(event);
        Integer stored = eventSizes.putIfAbsent(event, new Integer(eventSize));
        if (stored != null) {
                int storedSize = stored.intValue();
                if (eventSize != storedSize) {
                        LOGGER.debug(Thread.currentThread().getName() + " size of IoEvent changed from " + storedSize + " to " + eventSize);
                }
                eventSize = storedSize;
                eventSizes.remove(event);
        }
        return eventSize;
    }

1.3. Replace the call to estimateSize() inside offered() and polled() with a call to maintainSize()

If the state is logged and all sessions are somehow idle you could now see that the counter will return to it's initial value 0.



2. If the internal counter reaches or crosses the threshold *all* NioProcessor threads offering IoEvents will be stuck waiting for the lock. But when the counter drops below the threshold only *one* waiting thread will be notified.

Assume the session of the *one* continuing NioProcessor thread would have no more data transfer all then other threads would keep on waiting (NB: if you had specified an idle time for your server IoEvents could have notified all other waiting threads after a while hiding this problem).    

So I propose the following change: 

The unblock() method should call notifyAll() instead of notify().


Both problems together could mutually amplify to block forever.

I would appreciate any comments.

> Using IoEventQueueThrottler  with a WriteRequestFilter can lead to hangs
> ------------------------------------------------------------------------
>
>                 Key: DIRMINA-738
>                 URL: https://issues.apache.org/jira/browse/DIRMINA-738
>             Project: MINA
>          Issue Type: Bug
>          Components: Filter
>    Affects Versions: 2.0.0-M6
>            Reporter: Daniel John Debrunner
>             Fix For: 2.0.8
>
>
> The javadoc for WriteRequestFilter  shows an example using IoEventQueueThrottler  to throttle writes. First issue is that IoEventQueueThrottler  is not well documented, I'd assumed it was throttling number of messages, instead its threshold is number of bytes. Second issue is that if a message estimated length is less than the threshold then the write hangs, even with an async executor in the chain.
> Emmanuel Lécharny  also wrote:
>   FYI, the counter (threshold) is *never* decreased.
> To be frank, this is not a piece of code I know, but from what I see, it's *extremely* dubious that it does something useful, and most likely to block forever.
> I would suggest not to use this



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)