You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@qpid.apache.org by Gordon Sim <gs...@redhat.com> on 2014/05/08 22:04:21 UTC
Review Request 21234: QPID-5747: ensure that outgoing connection failure is
communicated to upper level
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21234/
-----------------------------------------------------------
Review request for qpid, Andrew Stitcher and Pavel Moravec.
Bugs: QPID-5747
https://issues.apache.org/jira/browse/QPID-5747
Repository: qpid
Description
-------
The failed callback passed in to Broker::connect() is only used until the TCP connection is established. The codec (i.e. qpid::broker::Connection object) is only created the first time the connection is writable. If the connection fails between establishing the tcp connection and determining that it is writable, then there is no way to communicate the failure.
This option fixes that by creating the codec object on disconnect if it hasn't already been created (and the connection is an outgoing one).
(Another option I think would be to create the codec for outgoing connections on AsyncHandler::init(), and add another flag to track whether the protocol header had been sent.)
Diffs
-----
/trunk/qpid/cpp/src/qpid/sys/AsynchIOHandler.cpp 1593267
Diff: https://reviews.apache.org/r/21234/diff/
Testing
-------
make test passes; original issue doesn't reproduce
Thanks,
Gordon Sim
Re: Review Request 21234: QPID-5747: ensure that outgoing connection failure
is communicated to upper level
Posted by Andrew Stitcher <as...@apache.org>.
> On May 12, 2014, 3:15 p.m., Andrew Stitcher wrote:
> > /trunk/qpid/cpp/src/qpid/sys/AsynchIOHandler.cpp, line 210
> > <https://reviews.apache.org/r/21234/diff/1/?file=577068#file577068line210>
> >
> > According to my preferred scheme:
> >
> > You'd get rid of the codec creation here, but instead assert:
> >
> > assert(codec);
A correction: this would actually need a new flag to say the connection has not been initialised.
Oh - I just noticed your original comments and see you preempted me, sorry for the noise!
- Andrew
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21234/#review42697
-----------------------------------------------------------
On May 8, 2014, 8:04 p.m., Gordon Sim wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21234/
> -----------------------------------------------------------
>
> (Updated May 8, 2014, 8:04 p.m.)
>
>
> Review request for qpid, Andrew Stitcher and Pavel Moravec.
>
>
> Bugs: QPID-5747
> https://issues.apache.org/jira/browse/QPID-5747
>
>
> Repository: qpid
>
>
> Description
> -------
>
> The failed callback passed in to Broker::connect() is only used until the TCP connection is established. The codec (i.e. qpid::broker::Connection object) is only created the first time the connection is writable. If the connection fails between establishing the tcp connection and determining that it is writable, then there is no way to communicate the failure.
>
> This option fixes that by creating the codec object on disconnect if it hasn't already been created (and the connection is an outgoing one).
>
> (Another option I think would be to create the codec for outgoing connections on AsyncHandler::init(), and add another flag to track whether the protocol header had been sent.)
>
>
> Diffs
> -----
>
> /trunk/qpid/cpp/src/qpid/sys/AsynchIOHandler.cpp 1593267
>
> Diff: https://reviews.apache.org/r/21234/diff/
>
>
> Testing
> -------
>
> make test passes; original issue doesn't reproduce
>
>
> Thanks,
>
> Gordon Sim
>
>
Re: Review Request 21234: QPID-5747: ensure that outgoing connection failure
is communicated to upper level
Posted by Andrew Stitcher <as...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21234/#review42697
-----------------------------------------------------------
I think the proposed code would work.
However I think it would be better to keep codec creation to a single location. Since we can't actually do that as server codec creation must happen after the first receive currently, having 2 locations is the next best thing. So I'd prefer to move the codec creation for client connections to the init method - I've put comments in the code where I think it should change.
/trunk/qpid/cpp/src/qpid/sys/AsynchIOHandler.cpp
<https://reviews.apache.org/r/21234/#comment76511>
In my preferred scheme codec creation for clients would go here:
See comment below.
/trunk/qpid/cpp/src/qpid/sys/AsynchIOHandler.cpp
<https://reviews.apache.org/r/21234/#comment76510>
According to my preferred scheme:
There should be an assert here:
assert(!isClient);
[Actually this should probably be here already]
/trunk/qpid/cpp/src/qpid/sys/AsynchIOHandler.cpp
<https://reviews.apache.org/r/21234/#comment76508>
Now that we would have multiple places to create the codec on a client type connection, I think it would be better to centralise this in AsynchIOHandler::init.
So this would work with
if (isclient) {
codec = factory->create(*this, identifier, getSecuritySettings(aio, nodict));
}
there. and some extra asserts (although the code here would now be unchanged)
/trunk/qpid/cpp/src/qpid/sys/AsynchIOHandler.cpp
<https://reviews.apache.org/r/21234/#comment76509>
According to my preferred scheme:
You'd get rid of the codec creation here, but instead assert:
assert(codec);
- Andrew Stitcher
On May 8, 2014, 8:04 p.m., Gordon Sim wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21234/
> -----------------------------------------------------------
>
> (Updated May 8, 2014, 8:04 p.m.)
>
>
> Review request for qpid, Andrew Stitcher and Pavel Moravec.
>
>
> Bugs: QPID-5747
> https://issues.apache.org/jira/browse/QPID-5747
>
>
> Repository: qpid
>
>
> Description
> -------
>
> The failed callback passed in to Broker::connect() is only used until the TCP connection is established. The codec (i.e. qpid::broker::Connection object) is only created the first time the connection is writable. If the connection fails between establishing the tcp connection and determining that it is writable, then there is no way to communicate the failure.
>
> This option fixes that by creating the codec object on disconnect if it hasn't already been created (and the connection is an outgoing one).
>
> (Another option I think would be to create the codec for outgoing connections on AsyncHandler::init(), and add another flag to track whether the protocol header had been sent.)
>
>
> Diffs
> -----
>
> /trunk/qpid/cpp/src/qpid/sys/AsynchIOHandler.cpp 1593267
>
> Diff: https://reviews.apache.org/r/21234/diff/
>
>
> Testing
> -------
>
> make test passes; original issue doesn't reproduce
>
>
> Thanks,
>
> Gordon Sim
>
>
Re: Review Request 21234: QPID-5747: ensure that outgoing connection failure
is communicated to upper level
Posted by Pavel Moravec <pm...@redhat.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21234/#review42674
-----------------------------------------------------------
Ship it!
The code change makes sense to me, though I am not too much familiar with AsynchIOHandler.
I run few tests relevant to original JIRA and everytime the link was attempting to reconnect as expected.
>From my side, ship it.
- Pavel Moravec
On May 8, 2014, 8:04 p.m., Gordon Sim wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21234/
> -----------------------------------------------------------
>
> (Updated May 8, 2014, 8:04 p.m.)
>
>
> Review request for qpid, Andrew Stitcher and Pavel Moravec.
>
>
> Bugs: QPID-5747
> https://issues.apache.org/jira/browse/QPID-5747
>
>
> Repository: qpid
>
>
> Description
> -------
>
> The failed callback passed in to Broker::connect() is only used until the TCP connection is established. The codec (i.e. qpid::broker::Connection object) is only created the first time the connection is writable. If the connection fails between establishing the tcp connection and determining that it is writable, then there is no way to communicate the failure.
>
> This option fixes that by creating the codec object on disconnect if it hasn't already been created (and the connection is an outgoing one).
>
> (Another option I think would be to create the codec for outgoing connections on AsyncHandler::init(), and add another flag to track whether the protocol header had been sent.)
>
>
> Diffs
> -----
>
> /trunk/qpid/cpp/src/qpid/sys/AsynchIOHandler.cpp 1593267
>
> Diff: https://reviews.apache.org/r/21234/diff/
>
>
> Testing
> -------
>
> make test passes; original issue doesn't reproduce
>
>
> Thanks,
>
> Gordon Sim
>
>
Re: Review Request 21234: QPID-5747: ensure that outgoing connection failure
is communicated to upper level
Posted by Andrew Stitcher <as...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21234/#review42830
-----------------------------------------------------------
Ship it!
I prefer this option to the first pass.
- Andrew Stitcher
On May 13, 2014, 3:34 p.m., Gordon Sim wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21234/
> -----------------------------------------------------------
>
> (Updated May 13, 2014, 3:34 p.m.)
>
>
> Review request for qpid, Andrew Stitcher and Pavel Moravec.
>
>
> Bugs: QPID-5747
> https://issues.apache.org/jira/browse/QPID-5747
>
>
> Repository: qpid
>
>
> Description
> -------
>
> The failed callback passed in to Broker::connect() is only used until the TCP connection is established. The codec (i.e. qpid::broker::Connection object) is only created the first time the connection is writable. If the connection fails between establishing the tcp connection and determining that it is writable, then there is no way to communicate the failure.
>
> This option fixes that by creating the codec object on disconnect if it hasn't already been created (and the connection is an outgoing one).
>
> (Another option I think would be to create the codec for outgoing connections on AsyncHandler::init(), and add another flag to track whether the protocol header had been sent.)
>
>
> Diffs
> -----
>
> /trunk/qpid/cpp/src/qpid/sys/AsynchIOHandler.h 1594220
> /trunk/qpid/cpp/src/qpid/sys/AsynchIOHandler.cpp 1594220
>
> Diff: https://reviews.apache.org/r/21234/diff/
>
>
> Testing
> -------
>
> make test passes; original issue doesn't reproduce
>
>
> Thanks,
>
> Gordon Sim
>
>
Re: Review Request 21234: QPID-5747: ensure that outgoing connection failure
is communicated to upper level
Posted by Gordon Sim <gs...@redhat.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21234/
-----------------------------------------------------------
(Updated May 13, 2014, 3:34 p.m.)
Review request for qpid, Andrew Stitcher and Pavel Moravec.
Changes
-------
This patch shows the alternative option. The codec for outgoing connections is created on establishing the connection, a new flag is added to track whether we have written out the protocol header for such connections. This means the codec is always available on disconnect.
Again, make test passes; original issue doesn't reproduce
Bugs: QPID-5747
https://issues.apache.org/jira/browse/QPID-5747
Repository: qpid
Description
-------
The failed callback passed in to Broker::connect() is only used until the TCP connection is established. The codec (i.e. qpid::broker::Connection object) is only created the first time the connection is writable. If the connection fails between establishing the tcp connection and determining that it is writable, then there is no way to communicate the failure.
This option fixes that by creating the codec object on disconnect if it hasn't already been created (and the connection is an outgoing one).
(Another option I think would be to create the codec for outgoing connections on AsyncHandler::init(), and add another flag to track whether the protocol header had been sent.)
Diffs (updated)
-----
/trunk/qpid/cpp/src/qpid/sys/AsynchIOHandler.h 1594220
/trunk/qpid/cpp/src/qpid/sys/AsynchIOHandler.cpp 1594220
Diff: https://reviews.apache.org/r/21234/diff/
Testing
-------
make test passes; original issue doesn't reproduce
Thanks,
Gordon Sim