You are viewing a plain text version of this content. The canonical link for it is here.
Posted to gitbox@activemq.apache.org by GitBox <gi...@apache.org> on 2021/05/05 11:41:49 UTC
[GitHub] [activemq-artemis] gtully opened a new pull request #3568: ARTEMIS-3200 - remove braces from the belt and braces fix in ARTEMIS-…
gtully opened a new pull request #3568:
URL: https://github.com/apache/activemq-artemis/pull/3568
…2712, the braces are not necessary and leak, cleaning up in close negates the need to the session closeable
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] clebertsuconic commented on pull request #3568: ARTEMIS-3200 - remove braces from the belt and braces fix in ARTEMIS-…
Posted by GitBox <gi...@apache.org>.
clebertsuconic commented on pull request #3568:
URL: https://github.com/apache/activemq-artemis/pull/3568#issuecomment-849819810
@gtully feel free to merge this whenever you get a full pass on the test suite.
All I care there is the test suite passing.. the changes are trivial however make tests pass there is some work there.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] gtully commented on pull request #3568: ARTEMIS-3200 - remove braces from the belt and braces fix in ARTEMIS-…
Posted by GitBox <gi...@apache.org>.
gtully commented on pull request #3568:
URL: https://github.com/apache/activemq-artemis/pull/3568#issuecomment-839815064
the closable go away when the connection closes, the issue/leak appears when there are repeated producer open/close on the same connection. It may need more tests that verify error conditions, I don't think those exist at the moment, there was one covering abort.
I will write some more tests and follow up with the commit.
At first look it seemed the closeable was just a catch all, and it hid the fact that the close was not always called. I think close should be called, b/c we already track the producer senders and we need to remove those in any event.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] gtully commented on pull request #3568: ARTEMIS-3200 - remove braces from the belt and braces fix in ARTEMIS-…
Posted by GitBox <gi...@apache.org>.
gtully commented on pull request #3568:
URL: https://github.com/apache/activemq-artemis/pull/3568#issuecomment-834203903
the instance seems to be properly tracked and close called, which does the cleanup. I don't see a reason for the callback registration on the session, which is the thing that leaks. tests look fine.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] gemmellr edited a comment on pull request #3568: ARTEMIS-3200 - remove braces from the belt and braces fix in ARTEMIS-…
Posted by GitBox <gi...@apache.org>.
gemmellr edited a comment on pull request #3568:
URL: https://github.com/apache/activemq-artemis/pull/3568#issuecomment-834579719
> @gemmellr can you expand on "since things dont need to the producer link rather than the parent session or connection"
>
> It looks like the large message file is "gathered" in the receiver, then handed off to the broker. The issue is when the send is aborted or errors out, in both cases, there is still a need for close in general to do cleanup.
Shoudl have said "dont need to _*close*_ the producer", i.e a client can close a session, without first closing a producer on it. Or a connection without first closing the sessions on it, or their producers or consumers.
Prior to your latest change here, I dont think closing the producer or session called the close method containing the cleanup call. Any cases that needed cleaned up perhaps only happened due to the registered callback thing (which is registered with a completely different object 'in the server' as opposed to 'in the amqp bits').
Now, after your change, an explicit session closure does look to call that, and explicit connection closure. Its not clear to me if it would be doing that on unexpected connection drop, havent seen that code. It perhaps did with the callback registered and doesnt without, or I don't think Clebert would have added it originally.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] gtully commented on pull request #3568: ARTEMIS-3200 - remove braces from the belt and braces fix in ARTEMIS-…
Posted by GitBox <gi...@apache.org>.
gtully commented on pull request #3568:
URL: https://github.com/apache/activemq-artemis/pull/3568#issuecomment-853937215
the tests are good, I think this is an improvement on what was there. it is a complex area but I think it is improved by having close be responsible for tidying up a pending file buffer considering the state of that producer is independent of the session. The broken scenario is client looping: create producer/send/close
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] clebertsuconic commented on a change in pull request #3568: ARTEMIS-3200 - remove braces from the belt and braces fix in ARTEMIS-…
Posted by GitBox <gi...@apache.org>.
clebertsuconic commented on a change in pull request #3568:
URL: https://github.com/apache/activemq-artemis/pull/3568#discussion_r649960211
##########
File path: artemis-protocols/artemis-amqp-protocol/src/main/java/org/apache/activemq/artemis/protocol/amqp/proton/ProtonAbstractReceiver.java
##########
@@ -285,13 +282,13 @@ protected void initializeCurrentLargeMessage(Delivery delivery, Receiver receive
@Override
public void close(boolean remoteLinkClose) throws ActiveMQAMQPException {
protonSession.removeReceiver(receiver);
+ clearLargeMessage();
Review comment:
@gtully can you change the whole method to removeReceiver and clear message upon remoteLinkClose=true?
false here means it was a detach and not a remote close.
I don't remember what a local detach would mean here.. but I guess it's better to be safe on this case.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] gemmellr commented on pull request #3568: ARTEMIS-3200 - remove braces from the belt and braces fix in ARTEMIS-…
Posted by GitBox <gi...@apache.org>.
gemmellr commented on pull request #3568:
URL: https://github.com/apache/activemq-artemis/pull/3568#issuecomment-834257058
Presumably the registration is meant for cases the close call to clearLargeMessage() doesnt happen, e.g when the session is closed without consumer close, or the connection drops etc...the latter is actually mentioned directly by Clebert on the original ARTEMIS-2712 JIRA, even though I had raised it to report a very separate issue with prior code changes not handling explicitly aborted deliveries and causing corruption of a subsequent delivery.
Actually, looking over the code now, it seems the close(ErrorCondition) method in ProtonAbstractReceiver that calls the clearLargeMessage() cleanup method doesnt look to be used at all. It seems like the code only calls that method for consumers (i.e ProtonServerSenderContext) e.g. when a queue is deleted, in AMQPSessionCallback.disconnect(ServerConsumer, SimpleString). If thats the case, it would seem such outstanding/incomplete 'large' file cleanup wont be occurring at all during consumer closure, and actually only later during session cleanup due to the registration that this PR is looking to remove?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] clebertsuconic commented on pull request #3568: ARTEMIS-3200 - remove braces from the belt and braces fix in ARTEMIS-…
Posted by GitBox <gi...@apache.org>.
clebertsuconic commented on pull request #3568:
URL: https://github.com/apache/activemq-artemis/pull/3568#issuecomment-839775353
@gtully I will have to trust you on this.. if you're sure the large message is cleared when the session / connection fails... go ahead and merge this.. (as long you have tested it.. it's good to go).
I thought those closeables would go away when the connection was closed... I certainly created an issue here :)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] clebertsuconic commented on pull request #3568: ARTEMIS-3200 - remove braces from the belt and braces fix in ARTEMIS-…
Posted by GitBox <gi...@apache.org>.
clebertsuconic commented on pull request #3568:
URL: https://github.com/apache/activemq-artemis/pull/3568#issuecomment-832746900
wouldn't be better to fix the leak?
Are you sure the clearLargeMessage would be called if you removed it?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] gemmellr commented on a change in pull request #3568: ARTEMIS-3200 - remove braces from the belt and braces fix in ARTEMIS-…
Posted by GitBox <gi...@apache.org>.
gemmellr commented on a change in pull request #3568:
URL: https://github.com/apache/activemq-artemis/pull/3568#discussion_r650020662
##########
File path: artemis-protocols/artemis-amqp-protocol/src/main/java/org/apache/activemq/artemis/protocol/amqp/proton/ProtonAbstractReceiver.java
##########
@@ -285,13 +282,13 @@ protected void initializeCurrentLargeMessage(Delivery delivery, Receiver receive
@Override
public void close(boolean remoteLinkClose) throws ActiveMQAMQPException {
protonSession.removeReceiver(receiver);
+ clearLargeMessage();
Review comment:
Based on the usage I believe false means it isnt in response to a 'client' asking for it via a detach frame, but is instead a 'local' close initiated on the server either directly (such as when administratively killing a consumer) or indirectly (e.g during connection drop, etc).
In the former case, the server could prvoke sending of a detach(closed=true) to the 'client', but there may still be message data in flight to the server before the client responds to the detach (again, assuming it does reply). If so, it would need to ensure it didnt handle any more payload if it is going to clear the file at this point. Or that it cleared it up later if it doesnt.
##########
File path: artemis-protocols/artemis-amqp-protocol/src/main/java/org/apache/activemq/artemis/protocol/amqp/proton/ProtonAbstractReceiver.java
##########
@@ -285,13 +282,13 @@ protected void initializeCurrentLargeMessage(Delivery delivery, Receiver receive
@Override
public void close(boolean remoteLinkClose) throws ActiveMQAMQPException {
protonSession.removeReceiver(receiver);
+ clearLargeMessage();
Review comment:
Based on the usage I believe false means it isnt in response to a 'client' asking for it via a detach frame, but is instead a 'local' close initiated on the server either directly (such as when administratively killing a producer) or indirectly (e.g during connection drop, etc).
In the former case, the server could prvoke sending of a detach(closed=true) to the 'client', but there may still be message data in flight to the server before the client responds to the detach (again, assuming it does reply). If so, it would need to ensure it didnt handle any more payload if it is going to clear the file at this point. Or that it cleared it up later if it doesnt.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] gtully commented on pull request #3568: ARTEMIS-3200 - remove braces from the belt and braces fix in ARTEMIS-…
Posted by GitBox <gi...@apache.org>.
gtully commented on pull request #3568:
URL: https://github.com/apache/activemq-artemis/pull/3568#issuecomment-853182907
some failures I saw on ci tests don't reproduce locally. I have pushed a rebase and will try another full run.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] gtully commented on pull request #3568: ARTEMIS-3200 - remove braces from the belt and braces fix in ARTEMIS-…
Posted by GitBox <gi...@apache.org>.
gtully commented on pull request #3568:
URL: https://github.com/apache/activemq-artemis/pull/3568#issuecomment-834277269
Indeed, I did some sanity in a debugger to verify close was being called via some of the tests but more tests may be necessary. Tracking state in close is natural and tracking producers/consumer in a session is also natural to trap the close. the callback registration looked unnatural to me and if it needs to be unregistered (as it does) it needs to be in close and if close is not called we will still have some potential problem.
let me see if I can write some more tests around this.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] asfgit closed pull request #3568: ARTEMIS-3200 - remove braces from the belt and braces fix in ARTEMIS-…
Posted by GitBox <gi...@apache.org>.
asfgit closed pull request #3568:
URL: https://github.com/apache/activemq-artemis/pull/3568
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] gemmellr commented on a change in pull request #3568: ARTEMIS-3200 - remove braces from the belt and braces fix in ARTEMIS-…
Posted by GitBox <gi...@apache.org>.
gemmellr commented on a change in pull request #3568:
URL: https://github.com/apache/activemq-artemis/pull/3568#discussion_r628335105
##########
File path: artemis-protocols/artemis-amqp-protocol/src/main/java/org/apache/activemq/artemis/protocol/amqp/proton/ProtonAbstractReceiver.java
##########
@@ -285,13 +282,13 @@ protected void initializeCurrentLargeMessage(Delivery delivery, Receiver receive
@Override
public void close(boolean remoteLinkClose) throws ActiveMQAMQPException {
protonSession.removeReceiver(receiver);
+ clearLargeMessage();
Review comment:
If remoteLinkClose is false, it possibly shouldnt clean up the message file just yet - more of, or possibly the entire remainder, of the delivery might still be in flight and actually arrive before the peers 'response' detach closure frame arrives (assuming any does). In that case the end of the incomplete message might get processed incorrectly as a corrupted message, potentially leaving another 'large file' unclosed, or worse being considered 'complete' and processed even though the start of it has been thrown away.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] gtully commented on pull request #3568: ARTEMIS-3200 - remove braces from the belt and braces fix in ARTEMIS-…
Posted by GitBox <gi...@apache.org>.
gtully commented on pull request #3568:
URL: https://github.com/apache/activemq-artemis/pull/3568#issuecomment-848724855
there is already a connection failure callback registered, the close(bool) was missing from the amqp callback, having that delegate to the amqp session context to give it a chance to close and cleanup seems to sort this. I think the session closeable can be safely removed now that close is in the mix in all cases. It is a bit of a web in there.
have kicked off a full test run to verify.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] gtully commented on pull request #3568: ARTEMIS-3200 - remove braces from the belt and braces fix in ARTEMIS-…
Posted by GitBox <gi...@apache.org>.
gtully commented on pull request #3568:
URL: https://github.com/apache/activemq-artemis/pull/3568#issuecomment-853937215
the tests are good, I think this is an improvement on what was there. it is a complex area but I think it is improved by having close be responsible for tidying up a pending file buffer considering the state of that producer is independent of the session. The broken scenario is client looping: create producer/send/close
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] gemmellr commented on pull request #3568: ARTEMIS-3200 - remove braces from the belt and braces fix in ARTEMIS-…
Posted by GitBox <gi...@apache.org>.
gemmellr commented on pull request #3568:
URL: https://github.com/apache/activemq-artemis/pull/3568#issuecomment-834579719
> @gemmellr can you expand on "since things dont need to the producer link rather than the parent session or connection"
>
> It looks like the large message file is "gathered" in the receiver, then handed off to the broker. The issue is when the send is aborted or errors out, in both cases, there is still a need for close in general to do cleanup.
A client can close a session, without first closing a producer on it. Or a connection without first closing the sessions on it, or their producers or consumers.
Prior to your latest change here, I dont think closing the producer or session called the close method containing the cleanup call. Any cases that needed cleaned up perhaps only happened due to the registered callback thing (which is registered with a completely different object 'in the server' as opposed to 'in the amqp bits').
Now, after your change, an explicit session closure does look to call that, and explicit connection closure. Its not clear to me if it would be doing that on unexpected connection drop, havent seen that code. It perhaps did with the callback registered and doesnt without, or I don't think Clebert would have added it originally.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] gemmellr edited a comment on pull request #3568: ARTEMIS-3200 - remove braces from the belt and braces fix in ARTEMIS-…
Posted by GitBox <gi...@apache.org>.
gemmellr edited a comment on pull request #3568:
URL: https://github.com/apache/activemq-artemis/pull/3568#issuecomment-834257058
Presumably the registration is meant for cases the close call to clearLargeMessage() doesnt happen, e.g when the session is closed without producer close, or the connection drops etc...the latter is actually mentioned directly by Clebert on the original ARTEMIS-2712 JIRA, even though I had raised it to report a very separate issue with prior code changes not handling explicitly aborted deliveries and causing corruption of a subsequent delivery.
Actually, looking over the code now, it seems the close(ErrorCondition) method in ProtonAbstractReceiver that calls the clearLargeMessage() cleanup method doesnt look to be used at all. It seems like the code only calls that method for consumers (i.e ProtonServerSenderContext) e.g. when a queue is deleted, in AMQPSessionCallback.disconnect(ServerConsumer, SimpleString). If thats the case, it would seem such outstanding/incomplete 'large' file cleanup wont be occurring at all during producer closure, and actually only later during session cleanup due to the registration that this PR is looking to remove?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] gemmellr commented on pull request #3568: ARTEMIS-3200 - remove braces from the belt and braces fix in ARTEMIS-…
Posted by GitBox <gi...@apache.org>.
gemmellr commented on pull request #3568:
URL: https://github.com/apache/activemq-artemis/pull/3568#issuecomment-834289173
The specific close method currently calling the cleanup doesnt looked to be used, whilst for the other close method (there are 2) there is no guarantee it will happen generally (though it will in the described case) since things dont need to the producer link rather than the parent session or connection, and things obviously drop, so relying on that close being called for all such cleanup is never going to be fullproof.
That said, the producer already looks to be registered with the session independent of that callback, so there are likely other ways to achieve that 'parent being cleaned up' triggered behaviour than with the callback which is being leaked. Or just change the registration so it doesnt retain the entire ProtonAbstractReceiver, rather just what it needs to close the outstanding file.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] gtully commented on pull request #3568: ARTEMIS-3200 - remove braces from the belt and braces fix in ARTEMIS-…
Posted by GitBox <gi...@apache.org>.
gtully commented on pull request #3568:
URL: https://github.com/apache/activemq-artemis/pull/3568#issuecomment-834539820
The cleanup needed to be in one of those closes, the one called by the other!
I think it is hanging together now, in that close does the work and the receiver is being tracked and close(bool) is being called.
@gemmellr can you expand on "since things dont need to the producer link rather than the parent session or connection"
It looks like the large message file is "gathered" in the receiver, then handed off to the broker. The issue is when the send is aborted or errors out, in both cases, there is still a need for close in general to do cleanup.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org