From 34161a3bc2b9917ccf9d51b7e995f52c69df55f4 Mon Sep 17 00:00:00 2001 From: Vincent Barbaresi Date: Tue, 31 Dec 2024 19:36:53 +0100 Subject: [PATCH] fix: stuck client when socket client never initializes (#676) There was an issue with the Client implementation: if a socket started a connection on the internal MJPEG port and was never initialized, the entire server thread would be stuck in the while loop waiting for initialization. This broke other clients connected to the server: no frames would be received anymore. To reproduce on emulator / real device: - start an Appium server with mJPEG server enabled (`appium:mjpegServerPort:9100` for instance) - Start an Appium session that reads the mJPEG frames (using Appium Inspector for instance) - Then from `adb shell`: connect to the internal mJPEG server (default port 7810): `nc localhost 7810` - --> Streaming will be broken for the existing clients This fixes the problem by returning early: if the client is not ready initialization will be retried later if necessary (or never) instead of blocking the thread Another side effect is that we actually receive frames from `nc localhost 7810` instead of it being stuck --- app/build.gradle | 2 + .../server/mjpeg/MjpegScreenshotClient.java | 4 +- .../server/MjpegScreenshotTest.java | 75 +++++++++++++++++++ 3 files changed, 80 insertions(+), 1 deletion(-) create mode 100644 app/src/test/java/io/appium/uiautomator2/server/MjpegScreenshotTest.java diff --git a/app/build.gradle b/app/build.gradle index 162bd2f94..800b23b48 100644 --- a/app/build.gradle +++ b/app/build.gradle @@ -63,6 +63,8 @@ android { '--add-exports', 'java.base/sun.nio.ch=ALL-UNNAMED', '--add-opens', 'java.base/java.lang.reflect=ALL-UNNAMED', '--add-opens', 'java.base/java.io=ALL-UNNAMED', + '--add-opens', 'java.base/java.net=ALL-UNNAMED', + '--add-opens', 'java.base/sun.net.www.protocol.http=ALL-UNNAMED', '--add-exports', 'jdk.unsupported/sun.misc=ALL-UNNAMED', ]) } diff --git a/app/src/main/java/io/appium/uiautomator2/server/mjpeg/MjpegScreenshotClient.java b/app/src/main/java/io/appium/uiautomator2/server/mjpeg/MjpegScreenshotClient.java index f3d2605d1..f5296d56e 100644 --- a/app/src/main/java/io/appium/uiautomator2/server/mjpeg/MjpegScreenshotClient.java +++ b/app/src/main/java/io/appium/uiautomator2/server/mjpeg/MjpegScreenshotClient.java @@ -102,8 +102,10 @@ void initialize() { } try { - while (!in.ready()) { + if (!in.ready()) { SystemClock.sleep(INPUT_NOT_READY_SLEEP_TIME_MS); + // Client initialization will be retried on the next frame + return; } Logger.info(String.format( diff --git a/app/src/test/java/io/appium/uiautomator2/server/MjpegScreenshotTest.java b/app/src/test/java/io/appium/uiautomator2/server/MjpegScreenshotTest.java new file mode 100644 index 000000000..5db41f3da --- /dev/null +++ b/app/src/test/java/io/appium/uiautomator2/server/MjpegScreenshotTest.java @@ -0,0 +1,75 @@ +package io.appium.uiautomator2.server.mjpeg; + +import org.junit.Test; +import org.junit.runner.RunWith; +import org.junit.After; +import org.junit.Before; +import static org.junit.Assert.*; + +import java.net.URL; +import java.net.HttpURLConnection; +import java.util.Collections; +import java.nio.charset.StandardCharsets; +import java.net.Socket; + +import static org.mockito.Mockito.spy; +import org.powermock.api.mockito.PowerMockito; +import org.powermock.core.classloader.annotations.PrepareForTest; +import org.powermock.modules.junit4.PowerMockRunner; + +import io.appium.uiautomator2.server.mjpeg.MjpegScreenshotServer; +import io.appium.uiautomator2.server.mjpeg.MjpegScreenshotStream; +import io.appium.uiautomator2.server.ServerConfig; + +@RunWith(PowerMockRunner.class) +@PrepareForTest(MjpegScreenshotStream.class) +public class MjpegScreenshotTest { + private static MjpegScreenshotServer serverThread; + private static int streamingPort; + + @Before + public void setUp() throws Exception { + // Create a MJPEG server with a mocked getScreenshot method + MjpegScreenshotStream mockScreenshotStreamSpy = + spy(new MjpegScreenshotStream(Collections.emptyList())); + byte[] mockScreenshotData = "screenshot data".getBytes(StandardCharsets.UTF_8); + PowerMockito.stub(PowerMockito.method(MjpegScreenshotStream.class, "getScreenshot")) + .toReturn(mockScreenshotData); + PowerMockito.whenNew(MjpegScreenshotStream.class) + .withAnyArguments() + .thenReturn(mockScreenshotStreamSpy); + + streamingPort = ServerConfig.DEFAULT_MJPEG_SERVER_PORT; + serverThread = new MjpegScreenshotServer(streamingPort); + serverThread.start(); + assertTrue(serverThread.isAlive()); + } + + @Test + public void shouldReceiveDataFromMjpegServer() throws Exception { + URL url = new URL(String.format("http://localhost:%d", streamingPort)); + HttpURLConnection connection = (HttpURLConnection) url.openConnection(); + connection.setRequestMethod("GET"); + int responseCode = connection.getResponseCode(); + assertEquals(HttpURLConnection.HTTP_OK, responseCode); + } + + @Test + public void shouldNotBlockOnUnInitializedClient() throws Exception { + // Create a socket and connect to the streaming server: it will create an uninitialized client + Socket socket = new Socket("localhost", streamingPort); + socket.close(); + + // We should still be able to receive data + URL url = new URL(String.format("http://localhost:%d", streamingPort)); + HttpURLConnection connection = (HttpURLConnection) url.openConnection(); + connection.setRequestMethod("GET"); + int responseCode = connection.getResponseCode(); + assertEquals(HttpURLConnection.HTTP_OK, responseCode); + } + + @After + public void tearDown() { + serverThread.interrupt(); + } +}