----------------------------------------------------------- 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 > >
