You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mina.apache.org by Emmanuel Lécharny <el...@gmail.com> on 2016/02/01 00:51:18 UTC
Session.close(true) issues
Hi guys,
following the bugs Radovan found last week, I'm now reviewing the
session.close(true) execution, and I have found some new problems. I
just create a JIRA ofr one of them (DIRMINA-1025).
Not that it's critical, but it's violating the contract : the session
should be closed immediately, but it's actually not.
The thing is that it might be strange to call close() explicitely in the
IoHandler, but there are many reasons you might want to do that.
Typically, when a exceptionCaught event has been trapped, but it may
also because we have received a message that is wrong (or simply a
message that requires the session to be closed).
As the process is purely asynchronous, we just change the status of the
session, and hope that every consecutive calls will check this status,
which is not the case :/ This has to be fixed. Typically, here is the
sequence of calls we can see :
process()
process reads
---> IoHandler.messageReceived()
// Do something
session.close()
<---
process writes
flush()
removeSessions()
As you can see, we could either process some writes, of flush some
pending messages, before the session is removed.
IMO, a calls to session.isClosing() should be added wherever we try to
do something on the session.
I have not yet analyzed the session( false ) call extensively, but I'm
quite sure some interesting things are going to be discovered ... Same
thing for the case where an exception is raised.
Re: Session.close(true) issues
Posted by Emmanuel Lécharny <el...@gmail.com>.
Le 01/02/16 00:51, Emmanuel Lécharny a écrit :
> Hi guys,
>
> following the bugs Radovan found last week, I'm now reviewing the
> session.close(true) execution, and I have found some new problems. I
> just create a JIRA ofr one of them (DIRMINA-1025).
>
> Not that it's critical, but it's violating the contract : the session
> should be closed immediately, but it's actually not.
>
> The thing is that it might be strange to call close() explicitely in the
> IoHandler, but there are many reasons you might want to do that.
> Typically, when a exceptionCaught event has been trapped, but it may
> also because we have received a message that is wrong (or simply a
> message that requires the session to be closed).
>
> As the process is purely asynchronous, we just change the status of the
> session, and hope that every consecutive calls will check this status,
> which is not the case :/ This has to be fixed. Typically, here is the
> sequence of calls we can see :
>
> process()
> process reads
> ---> IoHandler.messageReceived()
> // Do something
> session.close()
> <---
> process writes
> flush()
> removeSessions()
>
> As you can see, we could either process some writes, of flush some
> pending messages, before the session is removed.
>
> IMO, a calls to session.isClosing() should be added wherever we try to
> do something on the session.
>
>
> I have not yet analyzed the session( false ) call extensively, but I'm
> quite sure some interesting things are going to be discovered ... Same
> thing for the case where an exception is raised.
>
Some more investigations...
This is not a pleasant situation : my suggestion that
closeNow()/close(true) should throw an exception is just irrealistic.
The problem is that there are many parts in the stack where this
exception will be caught before being back to the original call in the
method where we process the reads :/ For instance in DefaultIoFilterChain :
private void callNextMessageReceived(Entry entry, IoSession session,
Object message) {
try {
IoFilter filter = entry.getFilter();
NextFilter nextFilter = entry.getNextFilter();
filter.messageReceived(nextFilter, session, message);
} catch (Exception e) {
fireExceptionCaught(e);
} catch (Error e) {
fireExceptionCaught(e);
throw e;
}
}
The only viable solution is to bubble up to the point where we process
the other operations, with the session in a specific state (closing).
Re: Session.close(true) issues
Posted by Emmanuel Lécharny <el...@gmail.com>.
Le 01/02/16 08:38, Emmanuel Lécharny a écrit :
> Le 01/02/16 00:51, Emmanuel Lécharny a écrit :
>> Hi guys,
>>
>> following the bugs Radovan found last week, I'm now reviewing the
>> session.close(true) execution, and I have found some new problems. I
>> just create a JIRA ofr one of them (DIRMINA-1025).
>>
>> Not that it's critical, but it's violating the contract : the session
>> should be closed immediately, but it's actually not.
>>
>> The thing is that it might be strange to call close() explicitely in the
>> IoHandler, but there are many reasons you might want to do that.
>> Typically, when a exceptionCaught event has been trapped, but it may
>> also because we have received a message that is wrong (or simply a
>> message that requires the session to be closed).
>>
>> As the process is purely asynchronous, we just change the status of the
>> session, and hope that every consecutive calls will check this status,
>> which is not the case :/ This has to be fixed. Typically, here is the
>> sequence of calls we can see :
>>
>> process()
>> process reads
>> ---> IoHandler.messageReceived()
>> // Do something
>> session.close()
>> <---
>> process writes
>> flush()
>> removeSessions()
>>
>> As you can see, we could either process some writes, of flush some
>> pending messages, before the session is removed.
>>
>> IMO, a calls to session.isClosing() should be added wherever we try to
>> do something on the session.
>>
>>
>> I have not yet analyzed the session( false ) call extensively, but I'm
>> quite sure some interesting things are going to be discovered ... Same
>> thing for the case where an exception is raised.
>>
> After a good night of sleep, I think that we should do two things :
> - deprecate the close(boolean) method, rename it closeNow() - standing
> for close(true)- and flushAndClose() - standing for close(false). The
> close() method will be remaped to flushAndClose(). That would clarify
> the purpose of teh close feature
> - when the closeNow() method is called, we should throw an exception
> (CloseException) that would be trapped in the IoProcessor loop, so that
> we can immediately kill the session, instead of potentially flushing
> some messages into the socket.
>
> wdyt ?
Side note : flushAndClose() is not the right name : we already have a
closeOnFlush() that is much better and do the job.
Re: Session.close(true) issues
Posted by Emmanuel Lécharny <el...@gmail.com>.
Le 01/02/16 00:51, Emmanuel Lécharny a écrit :
> Hi guys,
>
> following the bugs Radovan found last week, I'm now reviewing the
> session.close(true) execution, and I have found some new problems. I
> just create a JIRA ofr one of them (DIRMINA-1025).
>
> Not that it's critical, but it's violating the contract : the session
> should be closed immediately, but it's actually not.
>
> The thing is that it might be strange to call close() explicitely in the
> IoHandler, but there are many reasons you might want to do that.
> Typically, when a exceptionCaught event has been trapped, but it may
> also because we have received a message that is wrong (or simply a
> message that requires the session to be closed).
>
> As the process is purely asynchronous, we just change the status of the
> session, and hope that every consecutive calls will check this status,
> which is not the case :/ This has to be fixed. Typically, here is the
> sequence of calls we can see :
>
> process()
> process reads
> ---> IoHandler.messageReceived()
> // Do something
> session.close()
> <---
> process writes
> flush()
> removeSessions()
>
> As you can see, we could either process some writes, of flush some
> pending messages, before the session is removed.
>
> IMO, a calls to session.isClosing() should be added wherever we try to
> do something on the session.
>
>
> I have not yet analyzed the session( false ) call extensively, but I'm
> quite sure some interesting things are going to be discovered ... Same
> thing for the case where an exception is raised.
>
After a good night of sleep, I think that we should do two things :
- deprecate the close(boolean) method, rename it closeNow() - standing
for close(true)- and flushAndClose() - standing for close(false). The
close() method will be remaped to flushAndClose(). That would clarify
the purpose of teh close feature
- when the closeNow() method is called, we should throw an exception
(CloseException) that would be trapped in the IoProcessor loop, so that
we can immediately kill the session, instead of potentially flushing
some messages into the socket.
wdyt ?