[GitHub] qpid-proton issue #142: PROTON-1831: Incorrect open/close sequence for same ...

[alanconway]

Github user alanconway commented on the issue:

https://github.com/apache/qpid-proton/pull/142
  
Merged, see JIRA


---

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

[GitHub] qpid-proton issue #142: PROTON-1831: Incorrect open/close sequence for same ...

[alanconway]

Github user alanconway commented on the issue:

https://github.com/apache/qpid-proton/pull/142
  
The commit makes things better than they were but is not the final fix, as 
per comments from you and Gordon. Closing the PR as the JIRA has the comments - 
I think they are valid, just haven't got back to it yet.


---

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

[GitHub] qpid-proton issue #142: PROTON-1831: Incorrect open/close sequence for same ...

[gemmellr]

Github user gemmellr commented on the issue:

https://github.com/apache/qpid-proton/pull/142
  
It seems you already committed this before Gordon and I commented, but 
neglected to close the PR with the commit or otherwise. It either needs 
closing, or the commit reverted, depending what your intent is after the 
comments.


---

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

[GitHub] qpid-proton issue #142: PROTON-1831: Incorrect open/close sequence for same ...

[gemmellr]

Github user gemmellr commented on the issue:

https://github.com/apache/qpid-proton/pull/142
  
> Thanks - I didn't know about stolen. I think we could implement what the 
spec says on the server side - close the old link/handle with "stolen" and 
create a new link under the new handle.

In the same place its doing it now, or for the whole connection? (Still 
leaving other connections as something for the using server, e.g dispatch, to 
handle on its own).

Its worth saying that it doesnt currently check the handle is appropriate 
as it chokes on the name first. It'd need to check the handles more fully too, 
e.g two attaches with the same name and the same remote handle would still be 
an session:handle-in-use error.

> That would fix the crash, which was because of multiple handles to the 
same link object - we'd have a seprate link object for each handle, all but one 
of which are closed with "stolen". 

One further annoyance is how to inform the stuff already using the link 
that this all happened.

As I mentioned elsewhere before, this also starts getting into issues with 
limits checking e.g maxHandles (as it may still be 'in use' until the engine is 
processed later).

>I think on the client side its always an error to try to attach if you 
already have a link with same name an a handle. After disconnect all handles 
are cleared, so it is OK an error to re-attach during reconnect. I want to fix 
the client to refuse to create a link at all in this case. That would prevent 
this happening on the wire and give more immediate feedback to code that is 
mistakenly trying to double-attach. 

I think thats fair personally, always have.

> The fix is not trivial due to proton's batch-processing madness or I'd 
have done it first.

I feel your pain from previously looking at similar stuff :)


---

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

[GitHub] qpid-proton issue #142: PROTON-1831: Incorrect open/close sequence for same ...

[alanconway]

Github user alanconway commented on the issue:

https://github.com/apache/qpid-proton/pull/142
  
On Fri, Apr 27, 2018 at 4:57 AM, Robbie Gemmell 
wrote:

> The use of "invalid-field" for the error feels like it is maybe a little
> off. Its possible/likely there is nothing wrong with the information in 
the
> fields of the attach, as none of it is really actually checked (e.g 
whether
> its the same remote handle, itself a session:handle-in-use error) other
> than someone got there previously with the name. Perhaps "illegal-state"
> might be more applicable given the actual check being done?
>
> That said though, its not actually 100% clear it is an illegal state. The
> spec covers attaching an active link a second time as resulting in the
> first attachment being detached with the "stolen" link error. Its 
described
> for handling across different connections (something proton thus cant
> actually do on its own) e.g during reconnect, but its not clear that
> precludes it being the case on a single session or connection. However, it
> does seem far more likely to be in result of an error for the situation to
> ever occur on a single session/connection so perhaps treating it as such 
is
> the way to go in the end (plus any alternative would I guess be much 
harder
> to achieve, so maybe its the only choice for now).
>
Thanks - I didn't know about stolen. I think we could implement what the
spec says on the server side - close the old link/handle with "stolen" and
create a new link under the new handle. That would fix the crash, which was
because of multiple handles to the same  link object - we'd have a seprate
link object for each handle, all but one of which are closed with "stolen".

I think on the client side its always an error to try to attach if you
already have a link with same name an a handle. After disconnect all
handles are cleared, so it is OK an error to re-attach during reconnect.  I
want to fix the client to refuse to create a link at all in this case. That
would prevent this happening on the wire and give more immediate feedback
to code that is mistakenly trying to double-attach. The fix is not trivial
due to proton's batch-processing madness or I'd have done it first.

Does that sound right?
Cheers,
Alan.

> —
> You are receiving this because you authored the thread.
> Reply to this email directly, view it on GitHub
> ,
> or mute the thread
> 

> .
>



---

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

[GitHub] qpid-proton issue #142: PROTON-1831: Incorrect open/close sequence for same ...

[gemmellr]

Github user gemmellr commented on the issue:

https://github.com/apache/qpid-proton/pull/142
  
The use of "invalid-field" for the error feels like it is maybe a little 
off. Its possible/likely there is nothing wrong with the information in the 
fields of the attach, as none of it is really actually checked (e.g whether its 
the same remote handle, itself a session:handle-in-use error) other than 
someone got there previously with the name. Perhaps "illegal-state" might be 
more applicable given the actual check being done?

That said though, its not actually 100% clear it is an illegal state. The 
spec covers attaching an active link a second time as resulting in the first 
attachment being detached with the "stolen" link error. Its described for 
handling across different connections (something proton thus cant actually do 
on its own) e.g during reconnect, but its not clear that precludes it being the 
case on a single session or connection. However, it does seem far more likely 
to be in result of an error for the situation to ever occur on a single 
session/connection so perhaps treating it as such is the way to go in the end 
(plus any alternative would I guess be much harder to achieve, so maybe its the 
only choice for now).


---

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

[GitHub] qpid-proton issue #142: PROTON-1831: Incorrect open/close sequence for same ...

[codecov-io]

Github user codecov-io commented on the issue:

https://github.com/apache/qpid-proton/pull/142
  
# [Codecov](https://codecov.io/gh/apache/qpid-proton/pull/142?src=pr=h1) 
Report
> Merging 
[#142](https://codecov.io/gh/apache/qpid-proton/pull/142?src=pr=desc) into 
[master](https://codecov.io/gh/apache/qpid-proton/commit/19ceafa529a0432eee294200e6f6056f7817038a?src=pr=desc)
 will **increase** coverage by `0.06%`.
> The diff coverage is `93.54%`.

[![Impacted file tree 
graph](https://codecov.io/gh/apache/qpid-proton/pull/142/graphs/tree.svg?height=150=UKKzV9XnFF=pr=650)](https://codecov.io/gh/apache/qpid-proton/pull/142?src=pr=tree)

```diff
@@Coverage Diff @@
##   master #142  +/-   ##
==
+ Coverage   85.88%   85.94%   +0.06% 
==
  Files 236  236  
  Lines   3005630117  +61 
==
+ Hits2581325885  +72 
+ Misses   4243 4232  -11
```


| [Impacted 
Files](https://codecov.io/gh/apache/qpid-proton/pull/142?src=pr=tree) | 
Coverage Δ | |
|---|---|---|
| 
[c/src/core/transport.c](https://codecov.io/gh/apache/qpid-proton/pull/142/diff?src=pr=tree#diff-Yy9zcmMvY29yZS90cmFuc3BvcnQuYw==)
 | `90.49% <100%> (+0.02%)` | :arrow_up: |
| 
[c/tests/connection\_driver.c](https://codecov.io/gh/apache/qpid-proton/pull/142/diff?src=pr=tree#diff-Yy90ZXN0cy9jb25uZWN0aW9uX2RyaXZlci5j)
 | `98.8% <92.3%> (-1.2%)` | :arrow_down: |
| 
[c/src/core/codec.c](https://codecov.io/gh/apache/qpid-proton/pull/142/diff?src=pr=tree#diff-Yy9zcmMvY29yZS9jb2RlYy5j)
 | `81.93% <0%> (-0.32%)` | :arrow_down: |
| 
[c/tests/test\_tools.h](https://codecov.io/gh/apache/qpid-proton/pull/142/diff?src=pr=tree#diff-Yy90ZXN0cy90ZXN0X3Rvb2xzLmg=)
 | `86.88% <0%> (+14.75%)` | :arrow_up: |
| 
[c/tests/test\_handler.h](https://codecov.io/gh/apache/qpid-proton/pull/142/diff?src=pr=tree#diff-Yy90ZXN0cy90ZXN0X2hhbmRsZXIuaA==)
 | `100% <0%> (+15.94%)` | :arrow_up: |

--

[Continue to review full report at 
Codecov](https://codecov.io/gh/apache/qpid-proton/pull/142?src=pr=continue).
> **Legend** - [Click here to learn 
more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute  (impact)`, `ø = not affected`, `? = missing 
data`
> Powered by 
[Codecov](https://codecov.io/gh/apache/qpid-proton/pull/142?src=pr=footer). 
Last update 
[19ceafa...53b1768](https://codecov.io/gh/apache/qpid-proton/pull/142?src=pr=lastupdated).
 Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).



---

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