[jira] [Commented] (DIRMINA-845) ProtocolEncoderOutputImpl isn't thread-safe
[
https://issues.apache.org/jira/browse/DIRMINA-845?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13169893#comment-13169893
]
Dominic Williams commented on DIRMINA-845:
--
Ilya perhaps we can coordinate to create a solution for Red5.
Take a look at related issue regarding how serialization here can lead to
deadlock http://code.google.com/p/red5/issues/detail?id=164
> ProtocolEncoderOutputImpl isn't thread-safe
> ---
>
> Key: DIRMINA-845
> URL: https://issues.apache.org/jira/browse/DIRMINA-845
> Project: MINA
> Issue Type: Bug
> Components: Filter
>Affects Versions: 2.0.4
>Reporter: Ilya Ivanov
>
> ProtocolEncoderOutputImpl uses ConcurrentLinkedQueue and at first look it
> seems to be thread-safe. But really concurrent execution of flush method
> isn't thread-safe (and write-mergeAll also).
> E.g. in RTMP several channels multiplexed in single connection. According
> protocol specification it's possible to write to different channels
> concurrently. But it doesn't work with MINA.
> I've synchronized channel writing, but it doesn't prevent concurrent run of
> flushing (in 2.0.4 it's done directly in ProtocolCodecFilter.filterWrite, but
> ProtocolEncoderOutputImpl.flush has the same problem).
> Here the fragment of flushing code:
> while (!bufferQueue.isEmpty()) {
> Object encodedMessage = bufferQueue.poll();
>
> if (encodedMessage == null) {
> break;
> }
> // Flush only when the buffer has remaining.
> if (!(encodedMessage instanceof IoBuffer) || ((IoBuffer)
> encodedMessage).hasRemaining()) {
> SocketAddress destination = writeRequest.getDestination();
> WriteRequest encodedWriteRequest = new
> EncodedWriteRequest(encodedMessage, null, destination);
> nextFilter.filterWrite(session, encodedWriteRequest);
> }
> }
> Suppose original packets sequence is A, B, ...
> Concurrent run of flushing may proceed as following:
> thread-1: Object encodedMessage = bufferQueue.poll(); // gets A packet
> thread-2: Object encodedMessage = bufferQueue.poll(); // gets B packet
> ...
> thread-2: nextFilter.filterWrite(...); // writes B packet
> thread-1: nextFilter.filterWrite(...); // writes A packet
> so, resulting sequence will B, A
> It's quite confusing result especially when documentation doesn't contain any
> explanation about such behavior.
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators:
https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira
Re: [MINA 3] Filter's chain
On 12/14/11 2:17 PM, Julien Vermillard wrote: On Tue, Dec 13, 2011 at 5:58 PM, Emmanuel Lecharny wrote: Hi, the current implementation of filter's chain is associating this chain to a service. In MINA 2, each session had a copy of this chain. IMO, even if the default chain should be associated to the service, it would be a better idea to create a copy of this chain for each created session. the rational behind this choice is that we then be able to add or remove a filter from the chain dynamically for one specific session. One obvious usage could be to add a logger when one would want to get some traces for one session, and not for the whole service. We can also imagine some other usages. But it also has another direct advantage : currently, we compute the next filter to process in the DefaultIoFilterController, where an instance of the filter chain is stored. This DefaultIoFilterController is associated with a service, and can then be used by more than one selector, thus by more than one thread. That means we have to protect the current chain position for a given selector by storing it into a ThreadLocal. Access to the ThreadLocal values are way more expensive than accessing a session's current position if it's a simple integer. It would be easier if the controller was totally stateless, which would be possible if the session was holding the filter's chain. Now, that means we have to think about the way we should modify a chain for an existing session. Obviously, if we store the current position in the chain for a given session, we can't modify this position unless it's not 0, otherwise we might have some bad errors (ArrayOutOfBoundException, for instance). I still have to think a bit more about this, but I definitively think that it's a better approach. Thoughts ? Ok let's store de state (current chain position) in the session. that whould simplify the code (I'm not really happy with the thread local stuff) Ok will do that. -- Regards, Cordialement, Emmanuel Lécharny www.iktek.com
Re: [MINA 3] Idle session support
On Wed, Dec 14, 2011 at 2:10 PM, Emmanuel Lecharny wrote: > Hi, > > we discussed off line with Julien about Idle session support. Here is a sum > up of what we talked about, and we may have to make some choice. > > First of all, it's absolutely mandatory to be able to manage idle sessions, > in order to kill them, for instance. So we must offer a mechanism by which > the application developer can handle this. > > In MINA 2, if a session is idle for more than a given amount of time > (configurable), either on read, write or both, then an event is fired on the > chain, up to the handler. > > This is done in the selector loop. The problem with this approach is that > it's extremely expensive, and most certainly the worst possible solution to > deal with idle sessions, when it comes to manage hundred or thousands of > idle sessions. Another issue is that because it's really awfully costly, the > quantum between two detection is one second. Last, not least, this one > second os not very precise for a session, as if the session was created > between two checks, then the delay between the last check and the session > creation adds up. In other words, we can wait up to one more second to > detect that a session is idle. > > We have some better ideas : > - first, not all the sessions can be controlled. We need a list of session > registered for an idle check > - second, and more important, we must have a dedicated thread > - last, not least, this mechanism should be optional. Ie, when the session > is created, it's up to the application to tell that the session must be > checked for idleness, by registering it into the idleController. > > We should also allow a more fine grained control, like 100ms, and even, it > could be made configurable. > > thoughts ? > The current mechanism is pretty wrong, and cannot resolve idle time lesser than 1sec. We should do something not mandatory and efficient, I'm thinking about a time-period HashMap of list of session to check (we should update it for each session I/O events), and a timer checking every X ms the list of session in the map for this time index. Well I 'll try to clear my mind a propose a design. Julien
Re: [MINA 3] Filter's chain
On Tue, Dec 13, 2011 at 5:58 PM, Emmanuel Lecharny wrote: > Hi, > > the current implementation of filter's chain is associating this chain to a > service. In MINA 2, each session had a copy of this chain. > > IMO, even if the default chain should be associated to the service, it would > be a better idea to create a copy of this chain for each created session. > > the rational behind this choice is that we then be able to add or remove a > filter from the chain dynamically for one specific session. One obvious > usage could be to add a logger when one would want to get some traces for > one session, and not for the whole service. We can also imagine some other > usages. > > But it also has another direct advantage : currently, we compute the next > filter to process in the DefaultIoFilterController, where an instance of the > filter chain is stored. This DefaultIoFilterController is associated with a > service, and can then be used by more than one selector, thus by more than > one thread. That means we have to protect the current chain position for a > given selector by storing it into a ThreadLocal. Access to the ThreadLocal > values are way more expensive than accessing a session's current position if > it's a simple integer. > > It would be easier if the controller was totally stateless, which would be > possible if the session was holding the filter's chain. Now, that means we > have to think about the way we should modify a chain for an existing > session. Obviously, if we store the current position in the chain for a > given session, we can't modify this position unless it's not 0, otherwise we > might have some bad errors (ArrayOutOfBoundException, for instance). > > I still have to think a bit more about this, but I definitively think that > it's a better approach. > > Thoughts ? Ok let's store de state (current chain position) in the session. that whould simplify the code (I'm not really happy with the thread local stuff)
[MINA 3] Idle session support
Hi, we discussed off line with Julien about Idle session support. Here is a sum up of what we talked about, and we may have to make some choice. First of all, it's absolutely mandatory to be able to manage idle sessions, in order to kill them, for instance. So we must offer a mechanism by which the application developer can handle this. In MINA 2, if a session is idle for more than a given amount of time (configurable), either on read, write or both, then an event is fired on the chain, up to the handler. This is done in the selector loop. The problem with this approach is that it's extremely expensive, and most certainly the worst possible solution to deal with idle sessions, when it comes to manage hundred or thousands of idle sessions. Another issue is that because it's really awfully costly, the quantum between two detection is one second. Last, not least, this one second os not very precise for a session, as if the session was created between two checks, then the delay between the last check and the session creation adds up. In other words, we can wait up to one more second to detect that a session is idle. We have some better ideas : - first, not all the sessions can be controlled. We need a list of session registered for an idle check - second, and more important, we must have a dedicated thread - last, not least, this mechanism should be optional. Ie, when the session is created, it's up to the application to tell that the session must be checked for idleness, by registering it into the idleController. We should also allow a more fine grained control, like 100ms, and even, it could be made configurable. thoughts ? -- Regards, Cordialement, Emmanuel Lécharny www.iktek.com
