feat!: vendor DriverService and ExternalProcess for the local Appium service - #2460
Conversation
…service AppiumDriverLocalService and AppiumServiceBuilder no longer extend Selenium's DriverService and DriverService.Builder. The service was already final and overrode start/stop/isRunning/getUrl, so no shared base type is introduced. ExternalProcess is a trimmed copy of selenium-os's, and start() now throws IOException so a failed launch is reported as AppiumServerHasNotBeenStartedLocallyException rather than a raw UncheckedIOException. Free ports come from ServerSocket(0). selenium-os stays on the classpath transitively until the remote-driver dependency is removed. BREAKING CHANGE: AppiumDriverLocalService and AppiumServiceBuilder are no longer DriverService types. Removed getExecutable, setExecutable, sendOutputTo, getDriverProperty, getDriverEnvironmentVariable, score and withLogOutput. AppiumCommandExecutor constructors now take AppiumDriverLocalService instead of DriverService. close() on the service now stops the server. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
KazuCocoa
left a comment
KazuCocoa
left a comment
There was a problem hiding this comment.
One finding from reviewing the replacement process shutdown handling.
| try { | ||
| return process.waitFor(timeout.toMillis(), MILLISECONDS); | ||
| } catch (InterruptedException e) { | ||
| Thread.currentThread().interrupt(); |
There was a problem hiding this comment.
[P2] Defer restoring the interrupt flag until shutdown cleanup finishes. If the calling thread is already interrupted, or gets interrupted during the graceful-exit wait, restoring the flag here makes the subsequent wait after destroyForcibly() and both worker joins immediately throw InterruptedException. shutdown() can consequently return before the child has exited, and AppiumDriverLocalService.stop() then clears its process reference while the server may still hold its port. I reproduced this by starting /bin/sleep, interrupting the calling thread, and invoking shutdown(Duration.ofSeconds(5)): the child was still alive immediately after shutdown in 97 of 100 runs. Please preserve the interrupt state and restore it after the termination wait and worker cleanup have completed.
There was a problem hiding this comment.
Good catch, thanks. shutdown(Duration) now waits for the process exit and joins the output worker uninterruptibly, using a deadline loop that records any interrupt. It restores the interrupt status once, after cleanup has completed, so the child is always terminated before stop() drops its reference. I added a regression test that interrupts the calling thread before shutdown. It fails on the previous implementation and passes now. The public waitFor still aborts on interrupt, since there the caller's interrupt should win.
Restoring the interrupt flag inside the wait helpers made the wait after destroyForcibly() and both worker joins throw immediately, so shutdown() could return while the child was still alive and AppiumDriverLocalService would drop its reference to a server that still held the port. shutdown() now waits and joins uninterruptibly and restores the interrupt status once, after the termination and worker cleanup have completed. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
2 participants
AppiumDriverLocalService and AppiumServiceBuilder no longer extend Selenium's DriverService and DriverService.Builder. The service was already final and overrode start/stop/isRunning/getUrl, so no shared base type is introduced. ExternalProcess is a trimmed copy of selenium-os's, and start() now throws IOException so a failed launch is reported as AppiumServerHasNotBeenStartedLocallyException rather than a raw UncheckedIOException. Free ports come from ServerSocket(0).
selenium-os stays on the classpath transitively until the remote-driver dependency is removed.
BREAKING CHANGE: AppiumDriverLocalService and AppiumServiceBuilder are no longer DriverService types. Removed getExecutable, setExecutable, sendOutputTo, getDriverProperty, getDriverEnvironmentVariable, score and withLogOutput. AppiumCommandExecutor constructors now take AppiumDriverLocalService instead of DriverService. close() on the service now stops the server.