qpid-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "rajith attapattu" <rajit...@gmail.com>
Subject Re: Review Request: QPID-3604 - If connection is started and stopped, the client may get more messages than required by the prefetch value
Date Tue, 15 Nov 2011 22:07:36 GMT


> On 2011-11-15 16:41:28, Gordon Sim wrote:
> > http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQSession_0_10.java,
line 787
> > <https://reviews.apache.org/r/2832/diff/1/?file=58392#file58392line787>
> >
> >     Are there any other locks acquired as part of the block here? If so are there
any lock ordering issues where you could be introducing a deadlock?
> 
> rajith attapattu wrote:
>     Not that I could think of. The message-delivery-lock is taken to ensure that no messages
are being served while we start pulling them out of the queue.
>     In my tests so far, I haven't encountered any issues. However I plan to have more
manual tests - ex. Trying to stop the connection while the message consumers are in full flight.
> 
> Gordon Sim wrote:
>     What about the failover mutex? Could the release trigger a codepath that attempts
to acquire that? What about an asynchronous exception occurring concurrently; would that ever
need to acquire the message-delivery-lock?

Certainly possible as mentioned in the comment below. The failover and the synchronous exceptions
are things that could trigger a deadlock.
Testing is the best way to eliminate these possibilities. However IMO acquiring the message-delivery-lock
is a must to ensure unwanted interaction between messages delivery & releasing.


> On 2011-11-15 16:41:28, Gordon Sim wrote:
> > http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQSession_0_10.java,
line 796
> > <https://reviews.apache.org/r/2832/diff/1/?file=58392#file58392line796>
> >
> >     You are syncing here while holding the delivery lock, could that cause any problems?
> 
> rajith attapattu wrote:
>     So far I haven't encountered any issues. However things like failover, session exceptions
etc..could cause issues. I'm planning more thorough longer running tests.
>     Another thing I am considering is to not use a sync() at all. I'm not quite convinced
that it's of much value here.
>     
>     I've noticed that the client continues to get messages into it's queue even after
the code returns from the sync call. Hence the code snippet to release any messages received
after the connection is stopped. I was expecting the brokers response to the sync command
to be received after the client has got all the messages that were in flight. So after I sync
I could just release the messages in the queue and be done with it. But that's not the case.
>     
>     It seems that the dispatcher thread takes a bit of time to process the UnprocessedMessages
into the correct JMSMessages and put them onto the queue. So the sync() really doesn't add
much value here.
> 
> Gordon Sim wrote:
>     It sounds like it is necessary but not sufficient. You need to know that the stop
has been processed by the broker and it will not send any further, but you also need to synchronise
with the thread actually processing incoming messages(?).

I looked at it the other way :). Since it's not sufficient, it's not necessary. All though
it sounds wrong, not having the sync doesn't influence the outcome of the patch at all (Bcos
we release any messages we receive after the connection is stopped). 

It's not ideal, but with the absence of a proper mechanism to synchronize with the message
processing dispatcher thread this seems a reasonable approach.

Another reason why I shied away from attempting that was due to the nasty interactions we
may have with threading. The dispatcher thread does use the message delivery lock and that
route could increase the potential for a deadlock.


> On 2011-11-15 16:41:28, Gordon Sim wrote:
> > http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/BasicMessageConsumer_0_10.java,
line 126
> > <https://reviews.apache.org/r/2832/diff/1/?file=58393#file58393line126>
> >
> >     What case(s) is this code required for? You are releasing a message you have
just received, right? When is that required?
> 
> rajith attapattu wrote:
>     See the above for an explanation for why this is needed.
> 
> Gordon Sim wrote:
>     You mean this is here because of the lack of synchronization with the dispatcher
thread? If so that seems a little nasty to me... anyway to do this more cleanly?

That is precisely the reason. This also makes the sync call redundant. I started with the
sync() and realized that it wasn't sufficient, hence added this.
As explained above, I'm not sure if there is a reasonable way to synchronize with the message
delivery thread.

One possible approach might be is to do something like the syncDispatchQueue() method. Where
we push a certain marker message into the queue and then we get that we know there are no
more messages in the pipeline. But I'm concerned about the safety and feasibility of such
an approach.

Robbie I believe is one person who have looked at this code more extensively in the last little
while. So waiting to hear from him about his ideas as well. I'm open to suggestions on this
area. Lets see if we can collectively figure out a better solution.


- rajith


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/2832/#review3264
-----------------------------------------------------------


On 2011-11-15 15:36:36, rajith attapattu wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/2832/
> -----------------------------------------------------------
> 
> (Updated 2011-11-15 15:36:36)
> 
> 
> Review request for qpid, Gordon Sim, Robbie Gemmell, Weston Price, and Oleksandr Rudyy.
> 
> 
> Summary
> -------
> 
> This attempts to fix one of the issues related to the handling of Message credits. See
QPID-3602 for an overall picture of the various issues.
> 
> This particular patch does the following.
> 1. When the connection is stopped, it sends message.stop() & releases all messages
in the prefetch buffer.
> 2. It will also release any messages (that were in flight) that comes after the connection
is stopped. (*)
> 
> (*) This interferes with the immediate_prefetch feature. However I don't know if immediate
prefetch is really required in the 0-10 path.
> 
> As always comments, suggestions & criticisms are equally welcomed.
> 
> 
> This addresses bug QPID-3604.
>     https://issues.apache.org/jira/browse/QPID-3604
> 
> 
> Diffs
> -----
> 
>   http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQSession_0_10.java
1202228 
>   http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/BasicMessageConsumer_0_10.java
1202228 
>   http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/systests/src/main/java/org/apache/qpid/client/prefetch/PrefetchBehaviourTest.java
1202228 
>   http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQConnection.java
1202228 
> 
> Diff: https://reviews.apache.org/r/2832/diff
> 
> 
> Testing
> -------
> 
> See PrefetchBehaviourTest#testConnectionStop for more details.
> 
> 
> Thanks,
> 
> rajith
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message