[MINA 3] Session's state

[Emmanuel Lecharny <el...@gmail.com>]

Hi,

following the different states a session can be in, plus the possible 
transitions from one state to another, we will have an issue if we don't 
protect the sessions state against concurrent access and modifications.

For instance, as right now, the session's state is a volatile variable 
in AbstractIoSession :

     protected volatile SessionState state;

it looks like it's protected. It is, only of we consider it from a 
read/write pespective. That mean it's safe to read the sate, it's safe 
to change it, there is no way we can't do that as an atomic operation.

But there is something we can't do, it's changing the state *and* check 
that the transition is ok :

   if (state == CONNECTED ) {
     state = SECURING
   }

for instance, might fail as the session's state may have changed before 
we try to change it.

So we need to protect the state transition from concurrent accesses. 
Again, one possible solution would be to use a ReentrantReadWrite lock, 
to allow fast session's state reads, and safe transition.

I would also suggest that we have only one way to change the state, 
throw a method like :

void changeState( SessionState from, SessionState to)

in order to be able to check that the transition does not violate the 
table of possible transitions.

wdyt ?

-- 
Regards,
Cordialement,
Emmanuel Lécharny
www.iktek.com

Re: [MINA 3] Session's state

[sebb <se...@gmail.com>]

On 6 December 2011 18:26, Emmanuel Lecharny <el...@gmail.com> wrote:
> Hi,
>
> following the different states a session can be in, plus the possible
> transitions from one state to another, we will have an issue if we don't
> protect the sessions state against concurrent access and modifications.
>
> For instance, as right now, the session's state is a volatile variable in
> AbstractIoSession :
>
>    protected volatile SessionState state;
>
> it looks like it's protected. It is, only of we consider it from a
> read/write pespective. That mean it's safe to read the sate, it's safe to
> change it, there is no way we can't do that as an atomic operation.
>
> But there is something we can't do, it's changing the state *and* check that
> the transition is ok :
>
>  if (state == CONNECTED ) {
>    state = SECURING
>  }
>
> for instance, might fail as the session's state may have changed before we
> try to change it.
>
> So we need to protect the state transition from concurrent accesses. Again,
> one possible solution would be to use a ReentrantReadWrite lock, to allow
> fast session's state reads, and safe transition.
>
> I would also suggest that we have only one way to change the state, throw a
> method like :
>
> void changeState( SessionState from, SessionState to)

should be protected?

>
> in order to be able to check that the transition does not violate the table
> of possible transitions.
>
> wdyt ?

+1, but won't be totally safe unless the field is made private, and a
protected getter added.

If the field is not part of the public API, that would be OK;
otherwise it would not, as it breaks binary compat.

In any case, I suggest deprecating the field to document that it
should not be accessed directly.

> --
> Regards,
> Cordialement,
> Emmanuel Lécharny
> www.iktek.com
>

Re: [MINA 3] Session's state

[Chad Beaulac <ca...@gmail.com>]

+1 unit tests for all transitions ;-)

On Tue, Dec 6, 2011 at 3:05 PM, Julien Vermillard <jv...@gmail.com>wrote:

> On Tue, Dec 6, 2011 at 7:26 PM, Emmanuel Lecharny <el...@gmail.com>
> wrote:
> > Hi,
> >
> > following the different states a session can be in, plus the possible
> > transitions from one state to another, we will have an issue if we don't
> > protect the sessions state against concurrent access and modifications.
> >
> > For instance, as right now, the session's state is a volatile variable in
> > AbstractIoSession :
> >
> >    protected volatile SessionState state;
> >
> > it looks like it's protected. It is, only of we consider it from a
> > read/write pespective. That mean it's safe to read the sate, it's safe to
> > change it, there is no way we can't do that as an atomic operation.
> >
> > But there is something we can't do, it's changing the state *and* check
> that
> > the transition is ok :
> >
> >  if (state == CONNECTED ) {
> >    state = SECURING
> >  }
> >
> > for instance, might fail as the session's state may have changed before
> we
> > try to change it.
> >
> > So we need to protect the state transition from concurrent accesses.
> Again,
> > one possible solution would be to use a ReentrantReadWrite lock, to allow
> > fast session's state reads, and safe transition.
> >
> > I would also suggest that we have only one way to change the state,
> throw a
> > method like :
> >
> > void changeState( SessionState from, SessionState to)
> >
> > in order to be able to check that the transition does not violate the
> table
> > of possible transitions.
> >
> > wdyt ?
> >
>  + 1
> anyway I'm not sure it's handled correctly today (who said unit tests ?)
>

Re: [MINA 3] Session's state

[Julien Vermillard <jv...@gmail.com>]

On Tue, Dec 6, 2011 at 7:26 PM, Emmanuel Lecharny <el...@gmail.com> wrote:
> Hi,
>
> following the different states a session can be in, plus the possible
> transitions from one state to another, we will have an issue if we don't
> protect the sessions state against concurrent access and modifications.
>
> For instance, as right now, the session's state is a volatile variable in
> AbstractIoSession :
>
>    protected volatile SessionState state;
>
> it looks like it's protected. It is, only of we consider it from a
> read/write pespective. That mean it's safe to read the sate, it's safe to
> change it, there is no way we can't do that as an atomic operation.
>
> But there is something we can't do, it's changing the state *and* check that
> the transition is ok :
>
>  if (state == CONNECTED ) {
>    state = SECURING
>  }
>
> for instance, might fail as the session's state may have changed before we
> try to change it.
>
> So we need to protect the state transition from concurrent accesses. Again,
> one possible solution would be to use a ReentrantReadWrite lock, to allow
> fast session's state reads, and safe transition.
>
> I would also suggest that we have only one way to change the state, throw a
> method like :
>
> void changeState( SessionState from, SessionState to)
>
> in order to be able to check that the transition does not violate the table
> of possible transitions.
>
> wdyt ?
>
 + 1
anyway I'm not sure it's handled correctly today (who said unit tests ?)