[
https://issues.apache.org/jira/browse/QPID-3604?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13150827#comment-13150827
]
[email protected] commented on QPID-3604:
-----------------------------------------------------
bq. On 2011-11-15 16:41:28, Gordon Sim wrote:
bq. >
http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQSession_0_10.java,
line 787
bq. > <https://reviews.apache.org/r/2832/diff/1/?file=58392#file58392line787>
bq. >
bq. > 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?
bq.
bq. rajith attapattu wrote:
bq. 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.
bq. 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.
bq.
bq. Gordon Sim wrote:
bq. 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.
bq. On 2011-11-15 16:41:28, Gordon Sim wrote:
bq. >
http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQSession_0_10.java,
line 796
bq. > <https://reviews.apache.org/r/2832/diff/1/?file=58392#file58392line796>
bq. >
bq. > You are syncing here while holding the delivery lock, could that
cause any problems?
bq.
bq. rajith attapattu wrote:
bq. 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.
bq. 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.
bq.
bq. 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.
bq.
bq. 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.
bq.
bq. Gordon Sim wrote:
bq. 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.
bq. On 2011-11-15 16:41:28, Gordon Sim wrote:
bq. >
http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/BasicMessageConsumer_0_10.java,
line 126
bq. > <https://reviews.apache.org/r/2832/diff/1/?file=58393#file58393line126>
bq. >
bq. > What case(s) is this code required for? You are releasing a message
you have just received, right? When is that required?
bq.
bq. rajith attapattu wrote:
bq. See the above for an explanation for why this is needed.
bq.
bq. Gordon Sim wrote:
bq. 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:
bq.
bq. -----------------------------------------------------------
bq. This is an automatically generated e-mail. To reply, visit:
bq. https://reviews.apache.org/r/2832/
bq. -----------------------------------------------------------
bq.
bq. (Updated 2011-11-15 15:36:36)
bq.
bq.
bq. Review request for qpid, Gordon Sim, Robbie Gemmell, Weston Price, and
Oleksandr Rudyy.
bq.
bq.
bq. Summary
bq. -------
bq.
bq. 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.
bq.
bq. This particular patch does the following.
bq. 1. When the connection is stopped, it sends message.stop() & releases all
messages in the prefetch buffer.
bq. 2. It will also release any messages (that were in flight) that comes
after the connection is stopped. (*)
bq.
bq. (*) This interferes with the immediate_prefetch feature. However I don't
know if immediate prefetch is really required in the 0-10 path.
bq.
bq. As always comments, suggestions & criticisms are equally welcomed.
bq.
bq.
bq. This addresses bug QPID-3604.
bq. https://issues.apache.org/jira/browse/QPID-3604
bq.
bq.
bq. Diffs
bq. -----
bq.
bq.
http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQSession_0_10.java
1202228
bq.
http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/BasicMessageConsumer_0_10.java
1202228
bq.
http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/systests/src/main/java/org/apache/qpid/client/prefetch/PrefetchBehaviourTest.java
1202228
bq.
http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQConnection.java
1202228
bq.
bq. Diff: https://reviews.apache.org/r/2832/diff
bq.
bq.
bq. Testing
bq. -------
bq.
bq. See PrefetchBehaviourTest#testConnectionStop for more details.
bq.
bq.
bq. Thanks,
bq.
bq. rajith
bq.
bq.
> If the connection is stopped the client should release all it's messages in
> the prefetch buffer
> -----------------------------------------------------------------------------------------------
>
> Key: QPID-3604
> URL: https://issues.apache.org/jira/browse/QPID-3604
> Project: Qpid
> Issue Type: Bug
> Components: Java Client
> Affects Versions: 0.14
> Reporter: Rajith Attapattu
> Assignee: Rajith Attapattu
> Fix For: 0.15
>
>
> When connection.stop() is called, the JMS client should release all it's
> messages in the prefetch buffer.
> For all we know, the connection may never be started (depending on
> application logic) and those messages will be stuck on the prefetch buffer.
> Releasing it will allow another consumer to get them (in the case of a shared
> queue case).
> Another less severe but nevertheless an undesirable side affect of this is
> the client getting more messages than required by the capacity or prefetch
> arguments. See QPID-3602
> This may not be a big issue if the client is prefetching a few messages, but
> if prefetching something like 5000 messages, this could potentially cause a
> lethal spike in the clients memory usage.
> Even in low capacity/prefetch values, if the messages are large (say in the
> mega byte range) this could potentially put the client under memory pressure.
--
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
---------------------------------------------------------------------
Apache Qpid - AMQP Messaging Implementation
Project: http://qpid.apache.org
Use/Interact: mailto:[email protected]
