Skip to content

Commit b599bbd

Browse files
spikehmeta-codesync[bot]
spikeh
authored and
meta-codesync[bot]
committed
AsyncSocketTransport: use variant for mutually exclusive bindAddr/boundFd in connect()
Summary: In `AsyncSocketTransport::connect()`, `bindAddr` and `boundFd` are mutually exclusive. Make the intent clearer by using a `std::variant`. Reviewed By: dmm-fb Differential Revision: D94967832 fbshipit-source-id: 3bd9c95ce9cc06383211d18d7c0a63097f056388
1 parent 3a3167a commit b599bbd

14 files changed

Lines changed: 105 additions & 116 deletions

File tree

‎third-party/fizz/src/fizz/client/test/AsyncFizzClientTest.cpp‎

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -564,14 +564,13 @@ TEST_F(AsyncFizzClientTest, TestSocketConnectWithOpenSocket) {
564564
EventBase evb;
565565
MockAsyncSocket mockSocket(&evb);
566566
EXPECT_CALL(*socket_, getWrappedTransport()).WillOnce(Return(&mockSocket));
567-
EXPECT_CALL(mockSocket, connect_(_, _, _, _, _, _, _))
567+
EXPECT_CALL(mockSocket, connect_(_, _, _, _, _, _))
568568
.WillOnce(Invoke([](AsyncSocket::ConnectCallback* cb,
569569
const SocketAddress&,
570570
int,
571571
const SocketOptionMap&,
572-
const SocketAddress&,
573-
const std::string&,
574-
folly::NetworkSocket) {
572+
const AsyncSocketTransport::BindOptions&,
573+
const std::string&) {
575574
cb->connectErr(AsyncSocketException(
576575
AsyncSocketException::ALREADY_OPEN, "socket already open"));
577576
}));

‎third-party/folly/src/folly/io/async/AsyncIoUringSocket.cpp‎

Lines changed: 21 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -217,9 +217,8 @@ void AsyncIoUringSocket::connect(
217217
const folly::SocketAddress& address,
218218
std::chrono::milliseconds timeout,
219219
SocketOptionMap const& options,
220-
const folly::SocketAddress& bindAddr,
221-
const std::string& ifName,
222-
NetworkSocket boundFd) noexcept {
220+
const BindOptions& bindOptions,
221+
const std::string& ifName) noexcept {
223222
VLOG(4) << "AsyncIoUringSocket::connect() this=" << this << " to=" << address
224223
<< " fastopen=" << enableTFO_;
225224
evb_->dcheckIsInEventBaseThread();
@@ -230,33 +229,37 @@ void AsyncIoUringSocket::connect(
230229
connectSqe_ = std::make_unique<ConnectSqe>(this);
231230
}
232231
if (connectSqe_->inFlight()) {
233-
fileops::close(boundFd.toFd());
232+
if (auto* fd = std::get_if<NetworkSocket>(&bindOptions)) {
233+
fileops::close(fd->toFd());
234+
}
234235
callback->connectErr(AsyncSocketException(
235236
AsyncSocketException::NOT_OPEN, "connection in flight", -1));
236237
return;
237238
}
238239
if (fd_ != NetworkSocket{}) {
239-
fileops::close(boundFd.toFd());
240+
if (auto* fd = std::get_if<NetworkSocket>(&bindOptions)) {
241+
fileops::close(fd->toFd());
242+
}
240243
callback->connectErr(AsyncSocketException(
241244
AsyncSocketException::NOT_OPEN, "connection is connected", -1));
242245
return;
243246
}
244247
connectCallback_ = callback;
245248
peerAddress_ = address;
246249

247-
if (boundFd != NetworkSocket{}) {
250+
if (auto* boundFd = std::get_if<NetworkSocket>(&bindOptions)) {
248251
struct sockaddr_storage peerAddr{};
249252
socklen_t peerLen = sizeof(peerAddr);
250253
if (::getpeername(
251-
boundFd.toFd(),
254+
boundFd->toFd(),
252255
reinterpret_cast<struct sockaddr*>(&peerAddr),
253256
&peerLen) == 0) {
254-
fileops::close(boundFd.toFd());
257+
fileops::close(boundFd->toFd());
255258
callback->connectErr(AsyncSocketException(
256259
AsyncSocketException::INVALID_STATE, "boundFd is already connected"));
257260
return;
258261
}
259-
setFd(boundFd);
262+
setFd(*boundFd);
260263
} else {
261264
setFd(makeConnectSocket(address));
262265
}
@@ -285,8 +288,9 @@ void AsyncIoUringSocket::connect(
285288
return;
286289
}
287290

288-
// bind the socket, unless already provided
289-
if (bindAddr != anyAddress() && boundFd == NetworkSocket()) {
291+
// bind the socket
292+
if (auto* bindAddr = std::get_if<folly::SocketAddress>(&bindOptions);
293+
bindAddr && *bindAddr != anyAddress()) {
290294
sockaddr_storage addrStorage;
291295
auto saddr = reinterpret_cast<sockaddr*>(&addrStorage);
292296

@@ -299,14 +303,14 @@ void AsyncIoUringSocket::connect(
299303
// ports. Using the IP_BIND_ADDRESS_NO_PORT delays assigning a port until
300304
// connect expanding the available port range, unless
301305
// setBindAddressNoPort() is called.
302-
if (bindAddr.getPort() == 0) {
306+
if (bindAddr->getPort() == 0) {
303307
if (bindAddressNoPort_ &&
304308
setSockOpt(IPPROTO_IP, IP_BIND_ADDRESS_NO_PORT, &one, sizeof(one))) {
305309
auto errnoCopy = errno;
306310
callback->connectErr(AsyncSocketException(
307311
AsyncSocketException::NOT_OPEN,
308312
"failed to setsockopt IP_BIND_ADDRESS_NO_PORT prior to bind on " +
309-
bindAddr.describe(),
313+
bindAddr->describe(),
310314
errnoCopy));
311315
return;
312316
}
@@ -319,19 +323,19 @@ void AsyncIoUringSocket::connect(
319323
callback->connectErr(AsyncSocketException(
320324
AsyncSocketException::NOT_OPEN,
321325
"failed to setsockopt SO_REUSEADDR prior to bind on " +
322-
bindAddr.describe(),
326+
bindAddr->describe(),
323327
errnoCopy));
324328
return;
325329
}
326330
}
327331

328-
bindAddr.getAddress(&addrStorage);
332+
bindAddr->getAddress(&addrStorage);
329333

330-
if (::bind(fd_.toFd(), saddr, bindAddr.getActualSize()) != 0) {
334+
if (::bind(fd_.toFd(), saddr, bindAddr->getActualSize()) != 0) {
331335
auto errnoCopy = errno;
332336
callback->connectErr(AsyncSocketException(
333337
AsyncSocketException::NOT_OPEN,
334-
"failed to bind to async io_uring socket: " + bindAddr.describe(),
338+
"failed to bind to async io_uring socket: " + bindAddr->describe(),
335339
errnoCopy));
336340
return;
337341
}

‎third-party/folly/src/folly/io/async/AsyncIoUringSocket.h‎

Lines changed: 6 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -88,26 +88,23 @@ class AsyncIoUringSocket : public AsyncSocketTransport {
8888
const folly::SocketAddress& address,
8989
std::chrono::milliseconds timeout = std::chrono::milliseconds(0),
9090
SocketOptionMap const& options = emptySocketOptionMap,
91-
const SocketAddress& bindAddr = anyAddress(),
92-
const std::string& ifName = std::string(),
93-
NetworkSocket boundFd = NetworkSocket()) noexcept;
91+
const BindOptions& bindOptions = anyAddress(),
92+
const std::string& ifName = std::string()) noexcept;
9493

9594
void connect(
9695
ConnectCallback* callback,
9796
const folly::SocketAddress& address,
9897
int timeout,
9998
SocketOptionMap const& options,
100-
const SocketAddress& bindAddr,
101-
const std::string& ifName,
102-
NetworkSocket boundFd) noexcept override {
99+
const BindOptions& bindOptions,
100+
const std::string& ifName) noexcept override {
103101
connect(
104102
callback,
105103
address,
106104
std::chrono::milliseconds(timeout),
107105
options,
108-
bindAddr,
109-
ifName,
110-
boundFd);
106+
bindOptions,
107+
ifName);
111108
}
112109

113110
std::chrono::nanoseconds getConnectTime() const {

‎third-party/folly/src/folly/io/async/AsyncIoUringSocketFactory.h‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -67,7 +67,7 @@ class AsyncIoUringSocketFactory {
6767
* Create a socket bound to a source port that hashes to the ZC-RX queue.
6868
* Iterates ports in [1024, 32768) to find one that hashes to the target
6969
* queue and is available for binding. The returned socket can be passed as
70-
* the boundFd parameter to connect().
70+
* the BindOptions parameter to connect().
7171
*
7272
* Returns an invalid NetworkSocket on failure.
7373
*/

‎third-party/folly/src/folly/io/async/AsyncSSLSocket.cpp‎

Lines changed: 8 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -772,19 +772,17 @@ void AsyncSSLSocket::connect(
772772
const folly::SocketAddress& address,
773773
int timeout,
774774
const SocketOptionMap& options,
775-
const folly::SocketAddress& bindAddr,
776-
const std::string& ifName,
777-
NetworkSocket boundFd) noexcept {
775+
const BindOptions& bindOptions,
776+
const std::string& ifName) noexcept {
778777
auto timeoutChrono = std::chrono::milliseconds(timeout);
779778
connect(
780779
callback,
781780
address,
782781
timeoutChrono,
783782
timeoutChrono,
784783
options,
785-
bindAddr,
786-
ifName,
787-
boundFd);
784+
bindOptions,
785+
ifName);
788786
}
789787

790788
void AsyncSSLSocket::connect(
@@ -793,9 +791,8 @@ void AsyncSSLSocket::connect(
793791
std::chrono::milliseconds connectTimeout,
794792
std::chrono::milliseconds totalConnectTimeout,
795793
const SocketOptionMap& options,
796-
const folly::SocketAddress& bindAddr,
797-
const std::string& ifName,
798-
NetworkSocket boundFd) noexcept {
794+
const BindOptions& bindOptions,
795+
const std::string& ifName) noexcept {
799796
assert(!server_);
800797
assert(state_ == StateEnum::UNINIT);
801798
assert(sslState_ == STATE_UNINIT || sslState_ == STATE_UNENCRYPTED);
@@ -811,9 +808,8 @@ void AsyncSSLSocket::connect(
811808
address,
812809
int(connectTimeout.count()),
813810
options,
814-
bindAddr,
815-
ifName,
816-
boundFd);
811+
bindOptions,
812+
ifName);
817813
}
818814

819815
void AsyncSSLSocket::cancelConnect() {

‎third-party/folly/src/folly/io/async/AsyncSSLSocket.h‎

Lines changed: 4 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -463,9 +463,8 @@ class AsyncSSLSocket : public AsyncSocket {
463463
const folly::SocketAddress& address,
464464
int timeout = 0,
465465
const SocketOptionMap& options = emptySocketOptionMap,
466-
const folly::SocketAddress& bindAddr = anyAddress(),
467-
const std::string& ifName = "",
468-
NetworkSocket boundFd = NetworkSocket()) noexcept override;
466+
const BindOptions& bindOptions = anyAddress(),
467+
const std::string& ifName = "") noexcept override;
469468

470469
/**
471470
* A variant of connect that allows the caller to specify
@@ -488,9 +487,8 @@ class AsyncSSLSocket : public AsyncSocket {
488487
std::chrono::milliseconds connectTimeout,
489488
std::chrono::milliseconds totalConnectTimeout,
490489
const SocketOptionMap& options = emptySocketOptionMap,
491-
const folly::SocketAddress& bindAddr = anyAddress(),
492-
const std::string& ifName = "",
493-
NetworkSocket boundFd = NetworkSocket()) noexcept;
490+
const BindOptions& bindOptions = anyAddress(),
491+
const std::string& ifName = "") noexcept;
494492

495493
using AsyncSocket::connect;
496494

‎third-party/folly/src/folly/io/async/AsyncSocket.cpp‎

Lines changed: 18 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -897,17 +897,18 @@ void AsyncSocket::connect(
897897
const folly::SocketAddress& address,
898898
int timeout,
899899
const SocketOptionMap& options,
900-
const folly::SocketAddress& bindAddr,
901-
const std::string& ifName,
902-
NetworkSocket boundFd) noexcept {
900+
const BindOptions& bindOptions,
901+
const std::string& ifName) noexcept {
903902
DestructorGuard dg(this);
904903
eventBase_->dcheckIsInEventBaseThread();
905904

906905
addr_ = address;
907906

908907
// Make sure we're in the uninitialized state
909908
if (state_ != StateEnum::UNINIT) {
910-
netops_->close(boundFd);
909+
if (auto* fd = std::get_if<NetworkSocket>(&bindOptions)) {
910+
netops_->close(*fd);
911+
}
911912
return invalidState(callback);
912913
}
913914

@@ -925,19 +926,19 @@ void AsyncSocket::connect(
925926
auto saddr = reinterpret_cast<sockaddr*>(&addrStorage);
926927

927928
try {
928-
if (boundFd != NetworkSocket()) {
929+
if (auto* boundFd = std::get_if<NetworkSocket>(&bindOptions)) {
929930
struct sockaddr_storage peerAddr{};
930931
socklen_t peerLen = sizeof(peerAddr);
931932
if (netops_->getpeername(
932-
boundFd,
933+
*boundFd,
933934
reinterpret_cast<struct sockaddr*>(&peerAddr),
934935
&peerLen) == 0) {
935-
netops_->close(boundFd);
936+
netops_->close(*boundFd);
936937
throw AsyncSocketException(
937938
AsyncSocketException::INVALID_STATE,
938939
withAddr("boundFd is already connected"));
939940
}
940-
fd_ = boundFd;
941+
fd_ = *boundFd;
941942
} else {
942943
// Create the socket
943944
// Technically the first parameter should actually be a protocol family
@@ -1019,8 +1020,9 @@ void AsyncSocket::connect(
10191020
(void)ifName;
10201021
#endif
10211022

1022-
// bind the socket, unless already provided
1023-
if (bindAddr != anyAddress() && boundFd == NetworkSocket()) {
1023+
// bind the socket
1024+
if (auto* bindAddr = std::get_if<folly::SocketAddress>(&bindOptions);
1025+
bindAddr && *bindAddr != anyAddress()) {
10241026
int one = 1;
10251027
#if defined(IP_BIND_ADDRESS_NO_PORT) && !FOLLY_MOBILE && !defined(_WIN32) && \
10261028
!defined(__APPLE__)
@@ -1031,7 +1033,7 @@ void AsyncSocket::connect(
10311033
// ports. Using the IP_BIND_ADDRESS_NO_PORT delays assigning a port until
10321034
// connect expanding the available port range, unless
10331035
// setBindAddressNoPort() is called.
1034-
if (bindAddr.getPort() == 0) {
1036+
if (bindAddr->getPort() == 0) {
10351037
if (bindAddressNoPort_ &&
10361038
netops_->setsockopt(
10371039
fd_, IPPROTO_IP, IP_BIND_ADDRESS_NO_PORT, &one, sizeof(one))) {
@@ -1040,7 +1042,7 @@ void AsyncSocket::connect(
10401042
throw AsyncSocketException(
10411043
AsyncSocketException::NOT_OPEN,
10421044
"failed to setsockopt IP_BIND_ADDRESS_NO_PORT prior to bind on " +
1043-
bindAddr.describe(),
1045+
bindAddr->describe(),
10441046
errnoCopy);
10451047
}
10461048
} else {
@@ -1054,19 +1056,19 @@ void AsyncSocket::connect(
10541056
throw AsyncSocketException(
10551057
AsyncSocketException::NOT_OPEN,
10561058
"failed to setsockopt SO_REUSEADDR prior to bind on " +
1057-
bindAddr.describe(),
1059+
bindAddr->describe(),
10581060
errnoCopy);
10591061
}
10601062
}
10611063

1062-
bindAddr.getAddress(&addrStorage);
1064+
bindAddr->getAddress(&addrStorage);
10631065

1064-
if (netops_->bind(fd_, saddr, bindAddr.getActualSize()) != 0) {
1066+
if (netops_->bind(fd_, saddr, bindAddr->getActualSize()) != 0) {
10651067
auto errnoCopy = errno;
10661068
doClose();
10671069
throw AsyncSocketException(
10681070
AsyncSocketException::NOT_OPEN,
1069-
"failed to bind to async socket: " + bindAddr.describe(),
1071+
"failed to bind to async socket: " + bindAddr->describe(),
10701072
errnoCopy);
10711073
}
10721074
}

‎third-party/folly/src/folly/io/async/AsyncSocket.h‎

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -633,18 +633,18 @@ class AsyncSocket
633633
* @param timeout A timeout value, in milliseconds. If the connection
634634
* does not succeed within this period,
635635
* callback->connectError() will be invoked.
636-
* @param boundFd A socket with an address already bound to it via bind().
637-
* Ownership is transferred from the caller to this
638-
* AsyncSocket.
636+
* @param bindOptions Either a SocketAddress to bind to, or a NetworkSocket
637+
* with an address already bound to it via bind().
638+
* Ownership of a NetworkSocket is transferred from the
639+
* caller to this AsyncSocket.
639640
*/
640641
virtual void connect(
641642
ConnectCallback* callback,
642643
const folly::SocketAddress& address,
643644
int timeout = 0,
644645
const SocketOptionMap& options = emptySocketOptionMap,
645-
const folly::SocketAddress& bindAddr = anyAddress(),
646-
const std::string& ifName = "",
647-
NetworkSocket boundFd = NetworkSocket()) noexcept override;
646+
const BindOptions& bindOptions = anyAddress(),
647+
const std::string& ifName = "") noexcept override;
648648

649649
void connect(
650650
ConnectCallback* callback,

0 commit comments

Comments
 (0)