Re: Review Request 67139: treat attach after detach as new link

2018-05-16 Thread Alan Conway 
Misread it the first time. Ship It!

On Wed, May 16, 2018 at 10:01 AM, Robbie Gemmell 
wrote:

> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67139/
>
> Ship it!
>
> I think this seems fair, it the remote handle indicates an existing link 
> object was already detached it doesnt seem there would be reason to return 
> that while looking up something to indicate an attach for.
>
>
> - Robbie Gemmell
>
> On May 15th, 2018, 7:03 p.m. UTC, Gordon Sim wrote:
> Review request for qpid, Alan Conway, Ganesh Murthy, Justin Ross, Robbie
> Gemmell, and Ted Ross.
> By Gordon Sim.
>
> *Updated May 15, 2018, 7:03 p.m.*
> *Bugs: * PROTON-1845 
> *Repository: * qpid-proton-git
> Description
>
> If a peer sends attach, detach, attach with the same link name in the second 
> attach, the PN_LINK_REMOTE_OPEN event contains a reference to the link from 
> the first attach. This link is not in a state where it can be reused.
>
> It would be better to treat this as starting a new link object.
>
> This fixes DISPATCH-994 and DISPATCH-997
>
> Diffs
>
>- c/src/core/transport.c (e0a2b5c)
>
> View Diff 
>

Re: Review Request 67139: treat attach after detach as new link

2018-05-16 Thread Alan Conway 

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


Ship it!




Ship It!

- Alan Conway


On May 15, 2018, 7:03 p.m., Gordon Sim wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67139/
> ---
> 
> (Updated May 15, 2018, 7:03 p.m.)
> 
> 
> Review request for qpid, Alan Conway, Ganesh Murthy, Justin Ross, Robbie 
> Gemmell, and Ted Ross.
> 
> 
> Bugs: PROTON-1845
> https://issues.apache.org/jira/browse/PROTON-1845
> 
> 
> Repository: qpid-proton-git
> 
> 
> Description
> ---
> 
> If a peer sends attach, detach, attach with the same link name in the second 
> attach, the PN_LINK_REMOTE_OPEN event contains a reference to the link from 
> the first attach. This link is not in a state where it can be reused.
> 
> It would be better to treat this as starting a new link object.
> 
> This fixes DISPATCH-994 and DISPATCH-997
> 
> 
> Diffs
> -
> 
>   c/src/core/transport.c e0a2b5c 
> 
> 
> Diff: https://reviews.apache.org/r/67139/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Gordon Sim
> 
>

Re: Review Request 67139: treat attach after detach as new link

2018-05-16 Thread Robbie Gemmell 

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


Ship it!




I think this seems fair, it the remote handle indicates an existing link object 
was already detached it doesnt seem there would be reason to return that while 
looking up something to indicate an attach for.

- Robbie Gemmell


On May 15, 2018, 7:03 p.m., Gordon Sim wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67139/
> ---
> 
> (Updated May 15, 2018, 7:03 p.m.)
> 
> 
> Review request for qpid, Alan Conway, Ganesh Murthy, Justin Ross, Robbie 
> Gemmell, and Ted Ross.
> 
> 
> Bugs: PROTON-1845
> https://issues.apache.org/jira/browse/PROTON-1845
> 
> 
> Repository: qpid-proton-git
> 
> 
> Description
> ---
> 
> If a peer sends attach, detach, attach with the same link name in the second 
> attach, the PN_LINK_REMOTE_OPEN event contains a reference to the link from 
> the first attach. This link is not in a state where it can be reused.
> 
> It would be better to treat this as starting a new link object.
> 
> This fixes DISPATCH-994 and DISPATCH-997
> 
> 
> Diffs
> -
> 
>   c/src/core/transport.c e0a2b5c 
> 
> 
> Diff: https://reviews.apache.org/r/67139/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Gordon Sim
> 
>

Re: Review Request 67139: treat attach after detach as new link

2018-05-15 Thread Gordon Sim 

On 15/05/18 22:10, Alan Conway wrote:

Having trouble loggingn into reviewboard

I'm not sure if this is correct. If the link is not PN_LOCAL_CLOSED, then
the application is well within its rights to close() it.


I don't understand your point. This has no effect on what the 
application can do with any link objects it has. It only affects the 
link object that gets associated with a PN_LINK_REMOTE_OPEN event.



If we have re-used
it for another link that could close the wrong link.


Closing the link operates on a pn_link_t object the application created 
or got from an initial event. Where the peer sent an attach after a 
detach, this change causes the link object associated with the 
PN_LINK_REMOTE_OPEN event for the second attach to be a new link object, 
not the link object associated with the preceding detach, which is not 
in a usable state.



This would be the 3rd time we've changed this particular line of code in
the last few months,


I don't think so, git blame on master shows:

fd26ec66 proton-c/src/transport/transport.c (Gordon Sim 
2015-04-17 11:41:10 +0100 1268)



each time we find some new situation that fails. I'm
inclined to think we should never re-use a "dead" link, and instead find
out why proton is leaving these "dead" links lying around and fix that.


This change doesn't involve re-using a dead link. If there is a link 
with the same name that has been closed or detached by the peer it skips 
that link and therefore assumes a new link.


The change from the current code is that it *doesn't* try to reuse the 
'dead' link if the peer has closed (or detached) it, regardless of the 
local status. As the function ins only used for handling an incoming 
attach from the peer that seems correct to me.


-
To unsubscribe, e-mail: dev-unsubscr...@qpid.apache.org
For additional commands, e-mail: dev-h...@qpid.apache.org

Re: Review Request 67139: treat attach after detach as new link

2018-05-15 Thread Alan Conway 
Having trouble loggingn into reviewboard

I'm not sure if this is correct. If the link is not PN_LOCAL_CLOSED, then
the application is well within its rights to close() it. If we have re-used
it for another link that could close the wrong link.

This would be the 3rd time we've changed this particular line of code in
the last few months, each time we find some new situation that fails. I'm
inclined to think we should never re-use a "dead" link, and instead find
out why proton is leaving these "dead" links lying around and fix that.

On Tue, May 15, 2018 at 3:02 PM, Gordon Sim  wrote:

> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67139/
> Review request for qpid, Alan Conway, Ganesh Murthy, Justin Ross, and Ted
> Ross.
> By Gordon Sim.
> *Bugs: * PROTON-1845 
> *Repository: * qpid-proton-git
> Description
>
> If a peer sends attach, detach, attach with the same link name in the second 
> attach, the PN_LINK_REMOTE_OPEN event contains a reference to the link from 
> the first attach. This link is not in a state where it can be reused.
>
> It would be better to treat this as starting a new link object.
>
> This fixes DISPATCH-994 and DISPATCH-997
>
> Diffs
>
>- c/src/core/transport.c (e0a2b5c)
>
> View Diff 
>

Review Request 67139: treat attach after detach as new link

2018-05-15 Thread Gordon Sim 

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

Review request for qpid, Alan Conway, Ganesh Murthy, Justin Ross, and Ted Ross.


Bugs: PROTON-1845
https://issues.apache.org/jira/browse/PROTON-1845


Repository: qpid-proton-git


Description
---

If a peer sends attach, detach, attach with the same link name in the second 
attach, the PN_LINK_REMOTE_OPEN event contains a reference to the link from the 
first attach. This link is not in a state where it can be reused.

It would be better to treat this as starting a new link object.

This fixes DISPATCH-994 and DISPATCH-997


Diffs
-

  c/src/core/transport.c e0a2b5c 


Diff: https://reviews.apache.org/r/67139/diff/1/


Testing
---


Thanks,

Gordon Sim