Close AsyncServerSocket sockets in reverse order

Summary:
When a large number of processes concurrently bind and close `AsyncServerSocket`s with `port=0` (for example in tests) binding the IPv4 socket on the same port bound with the IPv6 socket can currently fail, because sockets are closed in the same order as they are opened, which makes the following scenario possible:

```
P0: close IPv6 port
P1: open IPv6 port (succeed)
P1: open IPv4 port (FAIL)
P0: close IPv4 port
```

This diff reverses the closing order, and also fixes a couple of outdated comments.

Reviewed By: philippv

Differential Revision: D2795920

fb-gh-sync-id: 0b5157a56dfed84aed4a590c103050a2727847b3

diff --git a/folly/io/async/AsyncServerSocket.cpp b/folly/io/async/AsyncServerSocket.cpp
index f7a395c080d79e3b624058fbd50d91b2f197a3c5..ba8a5d0a7b2ff2677e86925e8d737258de6ea4ca 100644 (file)
--- a/folly/io/async/AsyncServerSocket.cpp
+++ b/folly/io/async/AsyncServerSocket.cpp
@@ -181,10 +181,15 @@ int AsyncServerSocket::stopAccepting(int shutdownFlags) {
   }
   assert(eventBase_ == nullptr || eventBase_->isInEventBaseThread());
 
-  // When destroy is called, unregister and close the socket immediately
+  // When destroy is called, unregister and close the socket immediately.
   accepting_ = false;
 
-  for (auto& handler : sockets_) {
+  // Close the sockets in reverse order as they were opened to avoid
+  // the condition where another process concurrently tries to open
+  // the same port, succeed to bind the first socket but fails on the
+  // second because it hasn't been closed yet.
+  for (; !sockets_.empty(); sockets_.pop_back()) {
+    auto& handler = sockets_.back();
     handler.unregisterHandler();
     if (shutdownSocketSet_) {
       shutdownSocketSet_->close(handler.socket_);
@@ -195,7 +200,6 @@ int AsyncServerSocket::stopAccepting(int shutdownFlags) {
       closeNoInt(handler.socket_);
     }
   }
-  sockets_.clear();
 
   // Destroy the backoff timout.  This will cancel it if it is running.
   delete backoffTimeout_;
@@ -413,8 +417,7 @@ void AsyncServerSocket::bind(uint16_t port) {
     }
 
     // If port == 0, then we should try to bind to the same port on ipv4 and
-    // ipv6.  So if we did bind to ipv6, figure out that port and use it,
-    // except for the last attempt when we just use any port available.
+    // ipv6.  So if we did bind to ipv6, figure out that port and use it.
     if (sockets_.size() == 1 && port == 0) {
       SocketAddress address;
       address.setFromLocalAddress(sockets_.back().socket_);
@@ -430,9 +433,10 @@ void AsyncServerSocket::bind(uint16_t port) {
         }
       }
     } catch (const std::system_error& e) {
-      // if we can't bind to the same port on ipv4 as ipv6 when using port=0
-      // then we will try again another 2 times before giving up.  We do this
-      // by closing the sockets that were opened, then redoing the whole thing
+      // If we can't bind to the same port on ipv4 as ipv6 when using
+      // port=0 then we will retry again before giving up after
+      // kNumTries attempts.  We do this by closing the sockets that
+      // were opened, then restarting from scratch.
       if (port == 0 && !sockets_.empty() && tries != kNumTries) {
         for (const auto& socket : sockets_) {
           if (socket.socket_ <= 0) {
@@ -449,6 +453,7 @@ void AsyncServerSocket::bind(uint16_t port) {
         CHECK_EQ(0, getaddrinfo(nullptr, sport, &hints, &res0));
         continue;
       }
+
       throw;
     }