Hi Emmanuel,
On Mon, Feb 14, 2011 at 6:30 PM, Emmanuel Lecharny <[email protected]>wrote:
> On 2/15/11 2:35 AM, John Fallows wrote:
>
>> Folks,
>>
>
> Hi,
>
> first, if you think that you have found a bug, the best, really, is to
> create a JIRA. We can discuss the ins and outs of the problem in the JIRA,
> at least, nothing will get lost. The mailing list is on the opposite a
> perfect way to lose some informations :)
>
> Don't be afraid : if there is no bug, we simply close the JIRA, nothing
> more.
No problem - in the past I've seen folks want it confirmed on the list
before filing JIRA.
>
>
> We have discovered some blocking behavior in the Mina
>> AbstractPollingIoProcessor that is triggered while adding and removing
>> connections.
>>
> Adding a connection first triggers the Acceptor which create the socket and
> adds it to an IoProcessor. This is just a mmater of registering a Selection
> Key to the IoProcessor selector. As soon as the selectionKey is added, it
> generates an event which interrupts the worker select() call, then the newly
> added session is processed.
>
> Closing the connection does exactly the same thing : trigger an event and
> wake up the worker.
>
> This class manages an internal worker that must be started when the first
>> connection is added, and stopped when the last connection is removed.
>>
> The worker is started *before* the first connection reaches it, AFAICT. And
> when the last connection has been closed, the worker is not killed, unless
> you specifically requires its disposal. It just waits for new incoming
> connections.
>
> The code achieves this by using a synchronized block in startupProcessor()
>> as follows:
>>
>> public final void remove(T session) {
>> scheduleRemove(session);
>> startupProcessor();
>> }
>>
>> private void scheduleRemove(T session) {
>> removingSessions.add(session);
>> }
>>
>> private void startupProcessor() {
>> synchronized (lock) {
>> if (processor == null) {
>> processor = new Processor();
>> executor.execute(new NamePreservingRunnable(processor,
>> threadName));
>> }
>> }
>>
>> // Just stop the select() and start it again, so that the
>> processor
>> // can be activated immediately.
>> wakeup();
>> }
>>
>>
>> Each call to session.close() triggers the "filterClose" event on the
>> filter
>> chain, ending in a call to removeSession (shown above) where the
>> synchronized lock is obtained to verify that the processor is running in
>> order to close the connection. When a large number of connections are
>> closed at the same time, they will contend for the synchronized lock.
>>
> The lock is just used the time it takes to check for a value nullity. It's
> *extremely* fast. I would not say that there is some potential contention
> issue here. You'll probably need tens of thousands connection closing at the
> same time to see the consequences of such a lock.
Agreed. We did see significant impact when such collisions do occur.
>
>
> Similar behavior occurs when new connections are established via
>> addSession
>> (not shown here). Both removeSession and addSession synchronize on the
>> same
>> lock, so they also contend with each other as connections come and go.
>>
>
> I have to check the code to see if the IoProcessors can't be start *before*
> the Acceptor accepts incoming connections. That would eliminate the cost of
> creating new IoProcessors in a contended section of the code.
Yes, and a similar solution would also be needed for Connectors.
>
> If you agree that this is an issue and add it to your issue tracker, then
>> we
>> would be happy to upload a patch with a suggested solution using atomic
>> data
>> structures instead for your consideration.
>>
> Please, feel free to do so ! This is a community work, we don't expect to
> be best coders on earth !
>
> Note that we have found similar behavior in AbstractPollingIoAcceptor
>> and AbstractPollingIoConnector will gladly include suggested patches for
>> those as well.
>>
> Which makes sense, as they are symetrical.
>
> Thanks for your investigation, I'm waiting for the JIRA.
>
Sure thing - https://issues.apache.org/jira/browse/DIRMINA-819.
A patch is attached to DIRMINA-819 describing the proposed changes to
preserve the existing semantics using atomics and eliminate the lock.
Hope this is helpful,
-john.
--
>|< Kaazing Corporation >|<
John Fallows | CTO | +1.650.960.8148
444 Castro St, Suite 1100 | Mountain View, CA 94041, USA
