MINA
  1. MINA
  2. DIRMINA-738

Using IoEventQueueThrottler with a WriteRequestFilter can lead to hangs

    Details

    • Type: Bug Bug
    • Status: Open
    • Priority: Major Major
    • Resolution: Unresolved
    • Affects Version/s: 2.0.0-M6
    • Fix Version/s: None
    • Component/s: Filter
    • Labels:
      None

      Description

      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

        Activity

        Hide
        Markus Wimmer added a comment -

        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.

        Show
        Markus Wimmer added a comment - 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.
        Hide
        Emmanuel Lecharny added a comment -

        I'm looking at the IoEventQueueThrottle class, and there is something embarrassing about it : the polled() and offered() method are incrementing and decrementing the counter using an unreliable method : estimateSize().

        The estimateSize() method tries to compute the size of the message using reflection, but there is no guarantee whatsoever that the computed size reflects the actual message size. This seems a very brittle implementation, assuming that we could block the whole system if the threshold is reached.

        The problem is that the IoEventQueueThrottle is always used after the codec filter (there is no way we can efficiently use an Executor filter before the codec filter : we might not be able to correctly decode messages otherwise if the PDU is fragmented) so we never work on a IoBuffer...

        What you propose (ie matching the polled estimated size with the polled estimated size seems a better approach : at least, the size would be equals...

        I also do think that the notify() call could be replaced by a notifyAll() call, giving some room for more than a thread to be processed if a big chunk of memory has been released, when a call to notify() will only allow one single thread to proceed.

        I can apply the suggested modifications, but I don't have a test to reproduce the environment...

        Show
        Emmanuel Lecharny added a comment - I'm looking at the IoEventQueueThrottle class, and there is something embarrassing about it : the polled() and offered() method are incrementing and decrementing the counter using an unreliable method : estimateSize(). The estimateSize() method tries to compute the size of the message using reflection, but there is no guarantee whatsoever that the computed size reflects the actual message size. This seems a very brittle implementation, assuming that we could block the whole system if the threshold is reached. The problem is that the IoEventQueueThrottle is always used after the codec filter (there is no way we can efficiently use an Executor filter before the codec filter : we might not be able to correctly decode messages otherwise if the PDU is fragmented) so we never work on a IoBuffer... What you propose (ie matching the polled estimated size with the polled estimated size seems a better approach : at least, the size would be equals... I also do think that the notify() call could be replaced by a notifyAll() call, giving some room for more than a thread to be processed if a big chunk of memory has been released, when a call to notify() will only allow one single thread to proceed. I can apply the suggested modifications, but I don't have a test to reproduce the environment...

          People

          • Assignee:
            Unassigned
            Reporter:
            Daniel John Debrunner
          • Votes:
            2 Vote for this issue
            Watchers:
            2 Start watching this issue

            Dates

            • Created:
              Updated:

              Development