Repository navigation
fix: update executor and driver server URL state on directConnect - #2500
hoanle0126 wants to merge 1 commit into
Conversation
mykola-mokhnach
left a comment
mykola-mokhnach
left a comment
There was a problem hiding this comment.
- [minor]
ListensToSyslogMessages.java:69andListensToLogcatMessages.java:68aren't in the diff, but this change affects them. They choosewsorwssfromgetAddressOfRemoteServer(), which now switches tohttpsafter directConnect. Thehostandportarguments still come from the caller, usually the original hub.startSyslogBroadcast(hubHost, 4723)now openswss://hubHost:4723/...against a plain-WS port, where before this PR it usedws. Should these helpers also take the host and port from the updated URL, or should the scheme stay tied to the caller's endpoint?
| DirectConnectUrlSafety.requireSafeOverrideTarget(serverUrl); | ||
| HttpClient newClient = getHttpClientFactory().createClient(appiumClientConfig.baseUrl(serverUrl)); | ||
| HttpClient oldClient = this.client; | ||
| DirectConnectUrlSafety.requireSafeOverrideTarget(requireNonNull(serverUrl, "Server URL")); |
There was a problem hiding this comment.
[minor] appiumClientConfig, remoteServer and client are now three separate volatile writes. A thread calling getClient() or getAddressOfRemoteServer() during an override can see the new URL with the old client. volatile doesn't make the group atomic. remoteServer is also just appiumClientConfig.baseUrl() cached, which is the kind of duplicated state that caused this bug. Could getAddressOfRemoteServer() read appiumClientConfig.baseUrl() directly, and could the config and client sit in one immutable holder behind a single volatile reference (or this swap be synchronized)?
| public AppiumCommandExecutor(Map<String, AppiumCommandInfo> additionalCommands, | ||
| AppiumDriverLocalService service, AppiumClientConfig appiumClientConfig) { | ||
| this(additionalCommands, service, Factory.createDefault(), appiumClientConfig); | ||
| this(additionalCommands, requireNonNull(service), Factory.createDefault(), appiumClientConfig); |
There was a problem hiding this comment.
[minor] Adding requireNonNull(service) here changes behavior. The delegated 4-arg constructor accepts a null service (it wraps it in ofNullable), so a caller passing null to this overload used to get a working executor and now gets an NPE. Is that break intended? If so, it's worth a note in the PR description. It isn't part of the directConnect fix.
| */ | ||
| public URL getUrl() throws MalformedURLException { | ||
| String newUrlCandidate = String.format("%s://%s:%s%s", this.protocol, this.host, this.port, this.path); | ||
| String formattedHost = formatHost(this.host); |
There was a problem hiding this comment.
[nit] getUrl() formats the host and path before checking isValid(), only so the error message can include the candidate. That forces formatHost and normalizePath to handle null, and normalizePath returns the string "null". Checking isValid() first and formatting after would remove those placeholder branches.
| * @return the remote server URL, or {@code null} if not configured | ||
| */ | ||
| @Nullable | ||
| public URL getRemoteAddress() { |
There was a problem hiding this comment.
[nit] getRemoteAddress() now reads from the executor, so the remoteAddress field is only a fallback for a non-AppiumCommandExecutor. Both constructors only ever create or accept an AppiumCommandExecutor, so the field and the instanceof branch look dead. Dropping them would make it clear where the value comes from.
| Path tempDir; | ||
|
|
||
| private static final class RecordingHttpClient implements HttpClient { | ||
| private final ClientConfig config; |
There was a problem hiding this comment.
[minor] RecordingHttpClient.config is stored but never read. The overrideServerUrl test never checks that the replacement client was created with the overridden base URI, which was the original regression. Could it assert created.get(1).config.baseUri() equals the override URL, and the same for the preserved readTimeout? The drivers created in the new tests are also never quit.
Change list
AppiumCommandExecutor.overrideServerUrlto refresh the stored client configuration and remote server address alongside the HTTP client, and populate the base URL fromAppiumDriverLocalServicewhen omitted in the client configuration.AppiumDriver.getRemoteAddressto the active command executor so callers (includingSeleniumBridgeand log listeners) observe the updated server URL afterdirectConnector subsequent URL overrides.DirectConnect.Types of changes
Details
After the command executor became standalone in v11,
overrideServerUrlcreated a new HTTP client with the overridden base URL without updating the executor fields. That leftgetAddressOfRemoteServer(), the driver remote address, and the Selenium bridge client configuration pointing to the initial hub URL after direct connect.Added unit tests covering executor state updates, Selenium bridge client configuration propagation, local service base URL fallback, and IPv6 / path / blank-capability cases.