You are viewing a plain text version of this content. The canonical link for it is here.
Posted to proton@qpid.apache.org by gemmellr <gi...@git.apache.org> on 2015/04/20 21:01:58 UTC
[GitHub] qpid-proton pull request: PROTON-853: stop erroneous attach being ...
GitHub user gemmellr opened a pull request:
https://github.com/apache/qpid-proton/pull/21
PROTON-853: stop erroneous attach being sent
The changes for PROTON 154 commit 7d3063e7c488c97b9bad61e862d54b2b11dbc3d5 in 0.9 lead to situations where the transport will decide to send a new Attach frame out for a link that has been closed/detached.
The original change aimed to ensure that a link with the same name as one which had just been closed could be attached on the same session, by clearing the 'attach sent' and 'detach received' flags of the TransportLink. The reason that seemed necessary is that the old link object (and in turn TransportLink object) would be returned due to some caching within SessionImpl, unless you ensure to first call free() on the old link. Gordon noticed the same effect from the server side in proton-c via PROTON 850.
These changes stop the erroneous attach being sent by first reverting the change from PROTON 154, and looking to stop the earlier situation arising by returning a new Link object rather than a cached one if the states are closed (i.e similar to Gordons from PROTON 850), meaning a new TransportLink gets created rather than the old one being reused and there then being no need to play with the boolean states. It keeps a list of any 'old links' that would have been forogtten to ensure they get free'd when the session is.
The earlier test was ineffective and passed with or without the earlier changes. This was because it actually created new connections and sessions when it was aiming to be reuse the same session. I updated the test to do that, and it now catches both PROTON 154 and PROTON 850 (in proton-c too).
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/gemmellr/qpid-proton PROTON-853
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/qpid-proton/pull/21.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #21
----
commit 022ea2dbb7f5d7182fefb6e53adc8486777e69e0
Author: Robert Gemmell <ro...@apache.org>
Date: 2015-04-20T15:47:55Z
PROTON-853: revert the change from PROTON-154, including the test since it doesnt actually fail without the change.
This reverts commit 7d3063e7c488c97b9bad61e862d54b2b11dbc3d5.
commit 4b15a9b9befe727d2edde062e02da268aca40a83
Author: Robert Gemmell <ro...@apache.org>
Date: 2015-04-20T16:39:25Z
PROTON-853: add a test that catches the issue from PROTON-154 (and PROTON-850)
commit 00e69d3ad599beacba858f6ad1d4d75fb8c30fce
Author: Robert Gemmell <ro...@apache.org>
Date: 2015-04-20T16:41:10Z
PROTON-853: dont return the cached links if they are already in the closed state, instead create a new object and ensure the old links also get freed. Also fixes similar behaviour as in PROTON-850.
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] qpid-proton pull request: PROTON-853: stop erroneous attach being ...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/qpid-proton/pull/21
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] qpid-proton pull request: PROTON-853: stop erroneous attach being ...
Posted by gemmellr <gi...@git.apache.org>.
Github user gemmellr commented on the pull request:
https://github.com/apache/qpid-proton/pull/21#issuecomment-94542644
@rhs can you take a look? @dnwe can you also take a look to confirm if the changes work for you, e.g. handles any tests you have elsewhere based on needing the earlier changes?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] qpid-proton pull request: PROTON-853: stop erroneous attach being ...
Posted by dnwe <gi...@git.apache.org>.
Github user dnwe commented on the pull request:
https://github.com/apache/qpid-proton/pull/21#issuecomment-94719493
@gemmellr sure, just pulling in this PR now and will run some scenarios through it
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] qpid-proton pull request: PROTON-853: stop erroneous attach being ...
Posted by dnwe <gi...@git.apache.org>.
Github user dnwe commented on the pull request:
https://github.com/apache/qpid-proton/pull/21#issuecomment-95147399
+1 from me, not seen any issues in our FV after applying these commits + PROTON-850
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] qpid-proton pull request: PROTON-853: stop erroneous attach being ...
Posted by gemmellr <gi...@git.apache.org>.
Github user gemmellr commented on the pull request:
https://github.com/apache/qpid-proton/pull/21#issuecomment-94877763
Great. I'll add a test for the detach case as well in the morning.
Could you also look at PR 20 so I dont have to rebase my main repo to commit these? :P
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] qpid-proton pull request: PROTON-853: stop erroneous attach being ...
Posted by gemmellr <gi...@git.apache.org>.
Github user gemmellr commented on the pull request:
https://github.com/apache/qpid-proton/pull/21#issuecomment-94736378
Just a note to say, the new test will currently fail against proton-c just now, but Gordons fix from PROTON-850 resolves that. I mentioned this to him and he will push it in at some point.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] qpid-proton pull request: PROTON-853: stop erroneous attach being ...
Posted by rhs <gi...@git.apache.org>.
Github user rhs commented on the pull request:
https://github.com/apache/qpid-proton/pull/21#issuecomment-94755563
+1 from me
Initially I thought this wouldn't account for detach properly, but looking at the code I managed to convince myself that it will. If you have time, however, it would be nice to augment/add a test of the detach scenario.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---