Changeset 269128 in webkit

Timestamp:

Oct 28, 2020, 4:51:45 PM (6 years ago)

Author:

basuke.suzuki@sony.com

Message:

[WinCairo][PlayStation] Add handling for accept failure case
​https://bugs.webkit.org/show_bug.cgi?id=217353

Reviewed by Alex Christensen.

Source/JavaScriptCore:

It is rare to happen, but listening socket can be invalid state (i.e. cable disconnection, interface error),
and accept() will be called because of the poll's false report. In that situation, it is required to rebuild
the listening socket from the scratch. The failure of accept is the good place to capture this situation.

This patch moves listening duty into Listener internal calss and it is possible to make the invalid state
while maintained by SocketEndpoint. Also in case of failure continues, the retry will be gradually increasing
the intervals.

• inspector/remote/socket/RemoteInspectorServer.h:
• inspector/remote/socket/RemoteInspectorSocketEndpoint.cpp:

(Inspector::RemoteInspectorSocketEndpoint::listenInet):
(Inspector::RemoteInspectorSocketEndpoint::pollingTimeout):
(Inspector::RemoteInspectorSocketEndpoint::workerThread):
(Inspector::RemoteInspectorSocketEndpoint::createClient):
(Inspector::RemoteInspectorSocketEndpoint::disconnect):
(Inspector::RemoteInspectorSocketEndpoint::acceptInetSocketIfEnabled):

• inspector/remote/socket/RemoteInspectorSocketEndpoint.h:

Source/WebDriver:

Following the interface change.

• HTTPServer.h:
• socket/HTTPServerSocket.cpp:

(WebDriver::HTTPServer::didStatusChanged):

Location:

trunk/Source

Files:

• JavaScriptCore/ChangeLog (modified) (1 diff)
• JavaScriptCore/inspector/remote/socket/RemoteInspectorServer.h (modified) (1 diff)
• JavaScriptCore/inspector/remote/socket/RemoteInspectorSocketEndpoint.cpp (modified) (8 diffs)
• JavaScriptCore/inspector/remote/socket/RemoteInspectorSocketEndpoint.h (modified) (8 diffs)
• WebDriver/ChangeLog (modified) (1 diff)
• WebDriver/HTTPServer.h (modified) (1 diff)
• WebDriver/socket/HTTPServerSocket.cpp (modified) (1 diff)

trunk/Source/JavaScriptCore/ChangeLog

@@ +1 @@
+2020-10-28  Basuke Suzuki  <basuke.suzuki@sony.com>
+
+        [WinCairo][PlayStation] Add handling for accept failure case
+        https://bugs.webkit.org/show_bug.cgi?id=217353
+
+        Reviewed by Alex Christensen.
+
+        It is rare to happen, but listening socket can be invalid state (i.e. cable disconnection, interface error),
+        and accept() will be called because of the poll's false report. In that situation, it is required to rebuild
+        the listening socket from the scratch. The failure of accept is the good place to capture this situation.
+
+        This patch moves listening duty into Listener internal calss and it is possible to make the invalid state
+        while maintained by SocketEndpoint. Also in case of failure continues, the retry will be gradually increasing
+        the intervals.
+
+        * inspector/remote/socket/RemoteInspectorServer.h:
+        * inspector/remote/socket/RemoteInspectorSocketEndpoint.cpp:
+        (Inspector::RemoteInspectorSocketEndpoint::listenInet):
+        (Inspector::RemoteInspectorSocketEndpoint::pollingTimeout):
+        (Inspector::RemoteInspectorSocketEndpoint::workerThread):
+        (Inspector::RemoteInspectorSocketEndpoint::createClient):
+        (Inspector::RemoteInspectorSocketEndpoint::disconnect):
+        (Inspector::RemoteInspectorSocketEndpoint::acceptInetSocketIfEnabled):
+        * inspector/remote/socket/RemoteInspectorSocketEndpoint.h:
+
 2020-10-28  Saam Barati  <sbarati@apple.com>
 

trunk/Source/JavaScriptCore/inspector/remote/socket/RemoteInspectorServer.h

@@ -48 +48 @@
 
     Optional<ConnectionID> doAccept(RemoteInspectorSocketEndpoint&, PlatformSocketType) final;
-    void didClose(RemoteInspectorSocketEndpoint&, ConnectionID) final { };
+    void didChangeStatus(RemoteInspectorSocketEndpoint&, ConnectionID, RemoteInspectorSocketEndpoint::Listener::Status) final { };
 
     Optional<ConnectionID> m_server;

trunk/Source/JavaScriptCore/inspector/remote/socket/RemoteInspectorSocketEndpoint.cpp

@@ -32 +32 @@
 #include <wtf/MainThread.h>
 #include <wtf/RunLoop.h>
-#include <wtf/text/WTFString.h>
 
 namespace Inspector {
@@ -91 +90 @@
 Optional<ConnectionID> RemoteInspectorSocketEndpoint::listenInet(const char* address, uint16_t port, Listener& listener)
 {
-    if (auto socket = Socket::listen(address, port))
-        return createListener(*socket, listener);
-
-    return WTF::nullopt;
+    LockHolder lock(m_connectionsLock);
+    auto id = generateConnectionID();
+    auto connection = makeUnique<ListenerConnection>(id, listener, address, port);
+    if (!connection->isListening())
+        return WTF::nullopt;
+
+    m_listeners.add(id, WTFMove(connection));
+    wakeupWorkerThread();
+    return id;
 }
 
@@ -103 +107 @@
         return true;
     return false;
+}
+
+int RemoteInspectorSocketEndpoint::pollingTimeout()
+{
+    Optional<MonotonicTime> mostRecentWakeup;
+    for (const auto& connection : m_listeners) {
+        if (connection.value->nextRetryTime) {
+            if (mostRecentWakeup)
+                mostRecentWakeup = std::min<MonotonicTime>(*mostRecentWakeup, *connection.value->nextRetryTime);
+            else
+                mostRecentWakeup = connection.value->nextRetryTime;
+        }
+    }
+
+    if (mostRecentWakeup)
+        return static_cast<int>((*mostRecentWakeup - MonotonicTime::now()).milliseconds());
+
+    return -1;
 }
 
@@ -129 +151 @@
             }
             for (const auto& connection : m_listeners) {
-                pollfds.append(connection.value->poll);
-                ids.append(connection.key);
+                if (!connection.value->isListening() && connection.value->listen())
+                    connection.value->listener.didChangeStatus(*this, connection.key, Listener::Status::Listening);
+                if (connection.value->isListening()) {
+                    pollfds.append(connection.value->poll);
+                    ids.append(connection.key);
+                }
             }
         }
         pollfds.append(wakeup);
 
-        if (!Socket::poll(pollfds, -1))
+        if (!Socket::poll(pollfds, pollingTimeout()))
             continue;
 
@@ -181 +207 @@
     auto id = generateConnectionID();
     auto connection = makeUnique<ClientConnection>(id, socket, client);
+    if (!Socket::isValid(connection->socket))
+        return WTF::nullopt;
+
     m_clients.add(id, WTFMove(connection));
     wakeupWorkerThread();
@@ -195 +224 @@
         Socket::close(connection->socket);
         lock.unlockEarly();
-        connection->listener.didClose(*this, id);
+        connection->listener.didChangeStatus(*this, id, Listener::Status::Closed);
     } else if (const auto& connection = m_clients.get(id)) {
         m_clients.remove(id);
@@ -203 +232 @@
     } else
         LOG_ERROR("Error: Cannot disconnect: Invalid id");
-}
-
-Optional<ConnectionID> RemoteInspectorSocketEndpoint::createListener(PlatformSocketType socket, Listener& listener)
-{
-    ASSERT(Socket::isValid(socket));
-
-    if (!Socket::setup(socket))
-        return WTF::nullopt;
-
-    LockHolder lock(m_connectionsLock);
-    auto id = generateConnectionID();
-    auto connection = makeUnique<ListenerConnection>(id, socket, listener);
-    m_listeners.add(id, WTFMove(connection));
-    wakeupWorkerThread();
-
-    return id;
-}
-
-Optional<ConnectionID> RemoteInspectorSocketEndpoint::createListener(PlatformSocketType socket, Listener& listener, Client& client)
-{
-    ASSERT(Socket::isValid(socket));
-
-    if (!Socket::setup(socket))
-        return WTF::nullopt;
-
-    LockHolder lock(m_connectionsLock);
-    auto id = generateConnectionID();
-    auto connection = makeUnique<ListenerConnection>(id, socket, listener);
-    m_listeners.add(id, WTFMove(connection));
-    wakeupWorkerThread();
-
-    return id;
 }
 
@@ -360 +357 @@
             if (connection->listener.doAccept(*this, socket.value()))
                 return;
+
             Socket::close(*socket);
+        } else {
+            // If accept() returns error, we have to start over with bind() and listen().
+            // By closing socket here, listen() will be called again at the next loop of worker thread.
+            Socket::close(connection->socket);
+            connection->listener.didChangeStatus(*this, id, Listener::Status::Invalid);
         }
     }

trunk/Source/JavaScriptCore/inspector/remote/socket/RemoteInspectorSocketEndpoint.h

@@ -35 +35 @@
 #include <wtf/Threading.h>
 #include <wtf/Vector.h>
+#include <wtf/text/WTFString.h>
 
 namespace Inspector {
@@ -44 +45 @@
     public:
         virtual ~Client() { }
+
+        // These callbacks are not guaranteed to be called from the main thread.
         virtual void didReceive(RemoteInspectorSocketEndpoint&, ConnectionID, Vector<uint8_t>&&) = 0;
         virtual void didClose(RemoteInspectorSocketEndpoint&, ConnectionID) = 0;
@@ -50 +53 @@
     class Listener {
     public:
+        enum class Status : uint8_t {
+            Listening,
+            Invalid,
+            Closed,
+        };
         virtual ~Listener() { }
+
+        // These callbacks are not guaranteed to be called from the main thread.
         virtual Optional<ConnectionID> doAccept(RemoteInspectorSocketEndpoint&, PlatformSocketType) = 0;
-        virtual void didClose(RemoteInspectorSocketEndpoint&, ConnectionID) = 0;
+        virtual void didChangeStatus(RemoteInspectorSocketEndpoint&, ConnectionID, Status) = 0;
     };
 
@@ -70 +80 @@
 
     Optional<ConnectionID> createClient(PlatformSocketType, Client&);
-    Optional<ConnectionID> createListener(PlatformSocketType, Listener&, Client&);
 
     Optional<uint16_t> getPort(ConnectionID) const;
@@ -80 +89 @@
         WTF_MAKE_STRUCT_FAST_ALLOCATED;
 
-        BaseConnection(ConnectionID id, PlatformSocketType socket)
+        BaseConnection(ConnectionID id)
             : id { id }
-            , socket { socket }
-            , poll { Socket::preparePolling(socket) }
+            , socket { INVALID_SOCKET_VALUE }
         {
-            ASSERT(Socket::isValid(socket));
+        }
+
+        bool setSocket(PlatformSocketType newSocket)
+        {
+            ASSERT(Socket::isValid(newSocket));
+
+            if (!Socket::setup(newSocket))
+                return false;
+
+            if (Socket::isValid(socket))
+                Socket::close(socket);
+
+            socket = newSocket;
+            poll = Socket::preparePolling(socket);
+            return true;
         }
 
@@ -95 +117 @@
     struct ClientConnection : public BaseConnection {
         ClientConnection(ConnectionID id, PlatformSocketType socket, Client& client)
-            : BaseConnection(id, socket)
+            : BaseConnection(id)
             , client { client }
         {
+            setSocket(socket);
         }
 
@@ -105 +128 @@
 
     struct ListenerConnection : public BaseConnection {
-        ListenerConnection(ConnectionID id, PlatformSocketType socket, Listener& listener)
-            : BaseConnection(id, socket)
+        static constexpr Seconds initialRetryInterval { 200_ms };
+        static constexpr Seconds maxRetryInterval { 5_s };
+
+        ListenerConnection(ConnectionID id, Listener& listener, const char* address, uint16_t port)
+            : BaseConnection(id)
+            , address { address }
+            , port { port }
             , listener { listener }
         {
+            listen();
         }
 
+        bool listen()
+        {
+            ASSERT(!isListening());
+
+            if (nextRetryTime && *nextRetryTime > MonotonicTime::now())
+                return false;
+
+            if (auto newSocket = Socket::listen(address.utf8().data(), port)) {
+                if (setSocket(*newSocket)) {
+                    retryInterval = initialRetryInterval;
+                    return true;
+                }
+                Socket::close(*newSocket);
+            }
+
+            nextRetryTime = MonotonicTime::now() + retryInterval;
+            retryInterval = std::min<Seconds>(retryInterval * 2, maxRetryInterval);
+
+            return false;
+        }
+
+        bool isListening()
+        {
+            return Socket::isListening(socket);
+        }
+
+        String address;
+        uint16_t port;
         Listener& listener;
+        Optional<MonotonicTime> nextRetryTime;
+        Seconds retryInterval { initialRetryInterval };
     };
 
     ConnectionID generateConnectionID();
-    Optional<ConnectionID> createListener(PlatformSocketType, Listener&);
 
     void recvIfEnabled(ConnectionID);
@@ -123 +181 @@
     void acceptInetSocketIfEnabled(ConnectionID);
     bool isListening(ConnectionID);
+    int pollingTimeout();
 
     mutable Lock m_connectionsLock;

trunk/Source/WebDriver/ChangeLog

@@ +1 @@
+2020-10-28  Basuke Suzuki  <basuke.suzuki@sony.com>
+
+        [WinCairo][PlayStation] Add handling for accept failure case
+        https://bugs.webkit.org/show_bug.cgi?id=217353
+
+        Reviewed by Alex Christensen.
+
+        Following the interface change.
+
+        * HTTPServer.h:
+        * socket/HTTPServerSocket.cpp:
+        (WebDriver::HTTPServer::didStatusChanged):
+
 2020-10-22  Nitzan Uziely  <linkgoron@gmail.com>
 

trunk/Source/WebDriver/HTTPServer.h

@@ -96 +96 @@
 #if USE(INSPECTOR_SOCKET_SERVER)
     Optional<ConnectionID> doAccept(RemoteInspectorSocketEndpoint&, PlatformSocketType) final;
-    void didClose(RemoteInspectorSocketEndpoint&, ConnectionID) final;
+    void didChangeStatus(RemoteInspectorSocketEndpoint&, ConnectionID, RemoteInspectorSocketEndpoint::Listener::Status) final;
 #endif
 

trunk/Source/WebDriver/socket/HTTPServerSocket.cpp

@@ -57 +57 @@
 }
 
-void HTTPServer::didClose(RemoteInspectorSocketEndpoint&, ConnectionID)
+void HTTPServer::didChangeStatus(RemoteInspectorSocketEndpoint&, ConnectionID, RemoteInspectorSocketEndpoint::Listener::Status status)
 {
-    m_server = WTF::nullopt;
+    if (status == Status::Closed)
+        m_server = WTF::nullopt;
 }