----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/57392/#review168176 -----------------------------------------------------------
This looks good. It might have been simpler to just conditionalise the prototype creation though, rather than adding a new constructor. Alternatively it looks like creating the prototype could actually be done in SSLSocket::listen (which is in fact what the header comment says!). So it whould never get created for a client socket. Additionally actually defining ~SSLSocket() and calling PR_Close(prototype), might be good to remove the leak too. The Mozilla NSS doc says that config is copied from the prototype, so I don't think that the prototype needs to be kept alive longer than the listening socket. src/qpid/sys/ssl/SslSocket.cpp Line 125 (original), 130 (patched) <https://reviews.apache.org/r/57392/#comment240341> You could have simply changed this to: ```c++ if (certName.empty()) return; prototype = ... ... } ``` Or it is possible and meaningful to have an empty certName but check the client auth? - Andrew Stitcher On March 7, 2017, 7:42 p.m., Gordon Sim wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/57392/ > ----------------------------------------------------------- > > (Updated March 7, 2017, 7:42 p.m.) > > > Review request for qpid, Andrew Stitcher and Cliff Jansen. > > > Bugs: QPID-7693 > https://issues.apache.org/jira/browse/QPID-7693 > > > Repository: qpid-cpp > > > Description > ------- > > This avoids leaking a protoype socket for every client SslSocket created. (I > assume the prototype is still leaked for the server case, but since the > broker only closes the socket it listens on when shutting down that has much > less impact). This change just distinguishes sockets used for listening on > from those used for connecting. > > > Diffs > ----- > > src/qpid/sys/ssl/SslSocket.h 733a47a > src/qpid/sys/ssl/SslSocket.cpp 731151c > > > Diff: https://reviews.apache.org/r/57392/diff/1/ > > > Testing > ------- > > > Thanks, > > Gordon Sim > >
