Re: [beep4j-user] Concurrency issues

[Simon <co...@gm...>]

Hi

First of all thanks for your interest in beep4j. My comments are  
inlined below. However, first I have to tell you that I didn't have  
much time to work on beep4j recently. So it is kind of unmaintained  
(as you might have guessed from the activity statistics). However,  
0.1.1 seems to be successfully used by few people. Of course, it might  
depend on how the application uses beep4j. Anyways, I'm more than  
happy to incorporate patches and help you find your way around the  
beep4j codebase.

Cheers,
Simon

On May 5, 2008, at 9:08 AM, Thomson, Martin wrote:

> Hi,
>
> I'm seeing a few problems with beep4j under high loads in concurrent
> applications.
>
>
> ~ Out of order messages
>
> On inspection of the code in SessionImpl.java, it appears that out of
> order receipt of response messages causes a protocol exception (which
> drops the connection).  The replyListeners collection doesn't maintain
> the set of ReplyListener instances for each channel based on the  
> message
> number - the collection is a list.  When a response arrives it pulls  
> the
> first item of the list and raises the exception if the number doesn't
> match.
>
> One of the main advantages that BEEP provides is the out of order
> receipt of responses.  If this feature wasn't available, we might as
> well be using HTTP.

BEEP does provide out of order receipt of messages on different  
channels. However, replies on the same channel must be received in  
order. See section 2.6 of RFC 3080. The only exception (which I find  
somewhat strange) are frames of different ANSwers to the same MSG.

>
> I have a small patch that changes SessionImpl.java from 0.1.1 that  
> uses
> a Map rather than a List, i.e.
>    Map<Integer, Map<Integer, ReplyListenerHolder>> replyListeners
>
> I can send out that patch; however, it appears that there are a large
> number of changes between 0.1.1 and what is on the SVN trunk.  I am
> concerned that the changes don't fix this and could cause more  
> problems.
>
> It's difficult to follow the thread of execution; but based on a rough
> inspection, I see that the ReplyListener is now saved in
> FilterChainTargetHolder, which uses a ThreadLocal.  This would  
> present a
> problem if there were two requests sent from the same thread; the  
> second
> (and any subsequent requests from that thread) would overwrite the
> ReplyListener from the first.
>

The code in the trunk is really not tested that much. So it probably  
still contains lots of bugs. The main feature is filtering on channels  
(interceptors), well known from other specs (e.g. servlet filters).  
Additionally, the whole internals of beep4j were rewritten to work on  
frames instead of full assembled messages. This gives the receiver the  
chance to operate on frames (if he desires so) and so for example to  
terminate the session if the sender sends huges messages in an attempt  
to cause an OutOfMemoryError in the receiver.

The ThreadLocals must only be used in a synchronous call. i.e. when  
you send a message, the ReplyListener is put into the ThreadLocal and  
later picked up at the end of the filter chain. However, when the  
control returns to your application code, it should be reset and there  
should be no way you can override it, whether from the same thread or  
another one. Of course, it depends also on the used filters (i.e. a  
filter introducing asynchrony might cause troubles here if not  
properly implemented).

I believe that the trunk implementation should really be the  
implementation to be pushed.

>
> ~ Impact of congestion control for TCP mapping
>
> We are running some tests using a single BEEP channel to send a
> moderately high rate of messages between two applications.  Both are
> using beep4j.  Both do non-trivial processing on multiple threads.  We
> frequently get errors with seqno mismatches.
>
> The following (abridged) sequence is direct from a Wireshark packet
> capture:
>
> RPY 3 12 * 3784 312
> <!-- first part of reply to 3-12 -->
> END
> RPY 3 9 * 4257 0
> END
> SEQ 3 23232 4096
> RPY 3 9 . 4257 473
> <!-- reply to 3-9 -->
> END
> RPY 3 12 . 4096 161
> <!-- second part of reply to 3-12 -->
> END
> RPY 3 6 . 5203 473
> <!-- reply to 3-6 -->
> END
>
> As you can see - out of order message numbers on replies (I'm using  
> the
> SessionImpl.java patch mentioned above).  More alarming is the fact  
> that
> beep4j sends the first part of RPY 3-9 before finishing RPY 3-12.   
> What
> is causes the failure is the seqno mismatch it sees when the first
> (empty) part of RPY 3-9 is found.

Frames on the same channel must be sent in order. Does this also  
happen without your patch (which I think violates the BEEP  
specification, see above)?

>
>
> I think that the problem is that when there are two frames in the  
> queue
> and the sender window (i.e. that advertised by the other end) is too
> small, TCPMapping splits the first frame (correctly), puts the  
> remainder
> back on the queue (correctly), but then continues to try to process
> subsequent frames.  It should not be sending anything more (except  
> maybe
> the SEQ message) when it does not have any space left in the window.
>
> The following might fix that part:
>
> 	private Frame nextFrame() {
> +		if (frames.isEmpty() || senderWindow.remaining() <= 0) {
> -		if (frames.isEmpty()) {
> 		    return null;
>
> But I'm not sure what the calling context expects from sendFrames.  I
> expect that it should manage, but I'll need to test this further to  
> find
> out.
>

The whole frame splitting code in nextFrame() should not shuffle  
frames. If a frame is split, the first part is sent, the second part  
is added to the front of the queue and thus will be the first one to  
be sent the next time. From a quick glance at the code I do not see  
how this could happen. Could you send a unit test reproducing that  
problem on the ChannelHandler implementation.

>
> On the other hand, the following looks like a problem, but it might
> actually work.  When TCPMapping receives a SEQ message it immediately
> calls transport.sendBytes(createSEQFrame(channel, ackno, windowSize))
> rather than creating and queuing up a new Frame object.
>

When TCPMapping receives a SEQ frame it merely updates the send window  
size. Then, it tries to send queued frames. However, the SEQ frame is  
only created and sent after receiving ordinary frames (see  
frameReceived). Could you please clarify that one? Anyways, sending  
the SEQ frame directly might actually be a real problem (see below).

> I understand how this might be desirable, but it might be better (for
> the sake of interoperability) to defer the sending of this message if
> there is an outstanding split frame.  From RFC 3080:
>
>   When a message is segmented and sent as several frames, those frames
>   must be sent sequentially, without any intervening frames from other
>   messages on the same channel.
>
> I have a few partially formed thoughts on how to solve this, but I'll
> get back to that tomorrow.  I'll test the simple senderWindow check  
> once
> I have both fixes together.

Hm, that's right. I've never thought about the situation where a SEQ  
frame is interleaved between two frames of a message. So, a patch that  
solves the problem is very welcome.

>
>
> Let me know if anyone wants the SessionImpl.java patch for 0.1.1.
>
> Cheers,
> Martin
>
> ------------------------------------------------------------------------------------------------
> This message is for the designated recipient only and may
> contain privileged, proprietary, or otherwise private information.
> If you have received it in error, please notify the sender
> immediately and delete the original.  Any unauthorized use of
> this email is prohibited.
> ------------------------------------------------------------------------------------------------
> [mf2]
>
>
> -------------------------------------------------------------------------
> This SF.net email is sponsored by the 2008 JavaOne(SM) Conference
> Don't miss this year's exciting event. There's still time to save  
> $100.
> Use priority code J8TL2D2.
> http://ad.doubleclick.net/clk;198757673;13503038;p?http://java.sun.com/javaone
> _______________________________________________
> beep4j-user mailing list
> bee...@li...
> https://lists.sourceforge.net/lists/listinfo/beep4j-user