Dan Burkert has posted comments on this change. ( http://gerrit.cloudera.org:8080/9818 )
Change subject: KUDU-2351 Add IP/port for Recv() failure ...................................................................... Patch Set 11: (4 comments) http://gerrit.cloudera.org:8080/#/c/9818/11/src/kudu/security/tls_socket-test.cc File src/kudu/security/tls_socket-test.cc: http://gerrit.cloudera.org:8080/#/c/9818/11/src/kudu/security/tls_socket-test.cc@231 PS11, Line 231: threads.emplace_back(std::thread([&]{ Can you add some explanation for why this requires a background thread? Seems like you should just be able to call server.Stop() near line 241 between the two Recv() calls and it would work OK. If it does need to be done in the background then the way the cleanup is written is unsafe: 'server' only lives for the scope of this test. The thread is holding onto a reference to 'server' until TearDown() is executed, which will be a dangling reference. To make it safe you need to call join() before the end of the scope of this test. That being said, I don't see why join() is necessary at all. http://gerrit.cloudera.org:8080/#/c/9818/11/src/kudu/security/tls_socket.cc File src/kudu/security/tls_socket.cc: http://gerrit.cloudera.org:8080/#/c/9818/11/src/kudu/security/tls_socket.cc@111 PS11, Line 111: std::string kErrString = strings::Substitute("failed to read from TLS socket (remote: $0)", This is a really hot function, so the overhead of creating this message (allocation + the GetPeerAddress syscall) could end up showing up. I suggest making it lazy, so that it's only created on demand: auto err_string = [&] { Sockaddr remote; Socket::GetPeerAddress(&remote); std::string kErrString = strings::Substitute("failed to read from TLS socket (remote: $0)", }; and then change use-sites from 'kErrString' to 'err_string()'. http://gerrit.cloudera.org:8080/#/c/9818/11/src/kudu/security/tls_socket.cc@125 PS11, Line 125: "SSL_read error" probably worth prefixing this with the message as well. http://gerrit.cloudera.org:8080/#/c/9818/11/src/kudu/util/net/socket-test.cc File src/kudu/util/net/socket-test.cc: http://gerrit.cloudera.org:8080/#/c/9818/11/src/kudu/util/net/socket-test.cc@68 PS11, Line 68: sockaddr_promise.Set(sockaddr); Could this preamble be done in the main thread? Then you wouldn't need to use the promise. Seems like the if/else blocks are the only thing that needs to be done in the background. -- To view, visit http://gerrit.cloudera.org:8080/9818 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I22436b13bb351b132e1c0b7159294dd0c980c2b3 Gerrit-Change-Number: 9818 Gerrit-PatchSet: 11 Gerrit-Owner: Attila Bukor <[email protected]> Gerrit-Reviewer: Attila Bukor <[email protected]> Gerrit-Reviewer: Dan Burkert <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-Comment-Date: Wed, 18 Apr 2018 23:23:57 +0000 Gerrit-HasComments: Yes
