=== modified file 'src/com/goldencode/util/RandomWordGenerator.java' === modified file 'src/com/goldencode/p2j/security/HashPassword.java' === modified file 'src/com/goldencode/p2j/main/CreateAccountTask.java' === modified file 'src/com/goldencode/p2j/main/ClientSpawner.java' --- old/src/com/goldencode/p2j/main/ClientSpawner.java +++ new/src/com/goldencode/p2j/main/ClientSpawner.java @@ -102,6 +102,7 @@ import com.goldencode.p2j.net.SessionManager; import com.goldencode.p2j.net.RouterSessionManager; import com.goldencode.p2j.util.AppServerLauncher; +import com.goldencode.p2j.util.ConfigItem; import com.goldencode.p2j.util.appserver.*; import com.goldencode.p2j.util.logging.*; @@ -139,6 +140,151 @@ /** Timeout */ public static final int CLIENT_STARTUP_TIMEOUT_SEC = 30; + /** + * Smallest configured value not treated as a probable seconds-for-milliseconds mistake. The + * fastest client callback ever measured is over a second, so anything under this is far more + * likely to be a unit error than a deliberate setting. + */ + private static final long MIN_PLAUSIBLE_STARTUP_MS = 1000L; + + /** Largest configured value not treated as a probable unit mistake (one hour). */ + private static final long MAX_PLAUSIBLE_STARTUP_MS = 3_600_000L; + + /** + * Effective client-startup timeout in MILLISECONDS, resolved once from the directory. Volatile + * because the first spawn to arrive resolves it and every later thread reads it. + */ + private static volatile long clientStartupTimeoutMs = -1L; + + /** + * Milliseconds to wait for a spawned client to report ready. + *

+ * Read from the directory node {@code clientStartupTimeout} (web client section), in + * MILLISECONDS for consistency with the sibling {@code pollTimeout} and + * {@code connectionTestTimeout} nodes. When the node is absent the built-in + * {@link #CLIENT_STARTUP_TIMEOUT_SEC} seconds default applies. + *

+ *

+ * Resolved once and reported at WARNING so the value in force is never in doubt, and a value + * that looks like the wrong unit is called out rather than applied silently: reading 120 as + * milliseconds would destroy every client instantly, and 120000 as seconds would mean nothing + * was ever reaped for 33 hours. Both failures are near-impossible to diagnose from behaviour. + *

+ * + * @return The timeout in milliseconds. + */ + public static long getClientStartupTimeoutMs() + { + long cached = clientStartupTimeoutMs; + if (cached > 0L) + { + return cached; + } + + long resolved = CLIENT_STARTUP_TIMEOUT_SEC * 1000L; + String source = "built-in default (directory node 'clientStartupTimeout' absent)"; + try + { + Integer configured = ConfigItem.CLIENT_STARTUP_TIMEOUT.read(null); + if (configured != null && configured.intValue() > 0) + { + long ms = configured.longValue(); + if (ms < MIN_PLAUSIBLE_STARTUP_MS) + { + LOG.log(Level.SEVERE, + "[SPAWN-CFG] directory node 'clientStartupTimeout'=%d is in MILLISECONDS " + + "and %dms would destroy every client before it could start. This looks " + + "like seconds; did you mean %d? Using the built-in %ds default instead.", + configured, + configured, + Long.valueOf(ms * 1000L), + Integer.valueOf(CLIENT_STARTUP_TIMEOUT_SEC)); + } + else if (ms > MAX_PLAUSIBLE_STARTUP_MS) + { + LOG.log(Level.SEVERE, + "[SPAWN-CFG] directory node 'clientStartupTimeout'=%dms is over an hour, so " + + "a client that never starts would never be reaped. Applying it as asked, " + + "but check the unit -- this node is MILLISECONDS.", + configured); + resolved = ms; + source = "directory node 'clientStartupTimeout' (implausibly large)"; + } + else + { + resolved = ms; + source = "directory node 'clientStartupTimeout'"; + } + } + } + catch (Throwable t) + { + // A directory problem must not stop clients being spawned; fall back and say so. + LOG.log(Level.WARNING, + "[SPAWN-CFG] could not read 'clientStartupTimeout' from the directory, using " + + "the built-in %ds default", + Integer.valueOf(CLIENT_STARTUP_TIMEOUT_SEC), + t); + } + + clientStartupTimeoutMs = resolved; + LOG.log(Level.WARNING, + "[SPAWN-CFG] client startup timeout = %dms (%.1fs) (source: %s)", + Long.valueOf(resolved), + Double.valueOf(resolved / 1000.0), + source); + return resolved; + } + + /** + * Wait for the spawned client to report ready, and make an expiry impossible to miss. + *

+ * {@code CountDownLatch.await(timeout, unit)} returns false when the wait expired rather than + * the latch firing. Both call sites used to discard that boolean, so a client that never + * called back produced NO log at all -- the failure could only be inferred from a downstream + * symptom such as the browser being handed a dead port. It is now reported at SEVERE with the + * timeout that was applied and where that value came from. + *

+ * + * @param where + * Call site, so the two waits are distinguishable in the log. + * + * @return {@code true} if the client reported ready, {@code false} if the wait expired. + */ + private boolean awaitClientStartup(String where) + { + long timeoutMs = getClientStartupTimeoutMs(); + long start = System.nanoTime(); + boolean signalled = false; + try + { + signalled = clientStartupCountDown.await(timeoutMs, TimeUnit.MILLISECONDS); + } + catch (InterruptedException ignored) + { + LOG.log(Level.WARNING, + "[SPAWN-TIMEOUT] %s: interrupted after %dms while waiting for client startup", + where, + Long.valueOf((System.nanoTime() - start) / 1_000_000L)); + return false; + } + + if (!signalled) + { + // The event this whole investigation could not see. + LOG.log(Level.SEVERE, + "[SPAWN-TIMEOUT] CLIENT STARTUP TIMEOUT at %s: no ready callback within %dms " + + "(waited %dms) for osUser=%s -- the client is being DESTROYED even though it " + + "may still be booting. Raise the directory node 'clientStartupTimeout' " + + "(MILLISECONDS) if the client simply needs longer under load.", + where, + Long.valueOf(timeoutMs), + Long.valueOf((System.nanoTime() - start) / 1_000_000L), + getBuildParams().getOsUser()); + } + return signalled; + } + /** Logger */ private static final CentralLogger LOG = CentralLogger.get(ClientSpawner.class); @@ -152,6 +298,13 @@ private volatile boolean ready = false; /** + * System.nanoTime() at which the spawner process was forked, so clientIsReady() can report how + * long the client took to call back. Volatile: written by the spawning thread, read by the + * network thread that delivers the callback. + */ + private volatile long forkAtNanos = 0L; + + /** * Get {@link ClientBuilderParameters} used to spawn P2J clients. * * @return See above. @@ -323,6 +476,9 @@ int spawnerLaunchTimeout) throws IOException { + long tSpawnStart = System.nanoTime(); + long allocNanos = 0L; + if (allocator != null) { ClientBuilderParameters buildParams = getBuildParams(); @@ -330,15 +486,26 @@ "localhost", buildParams.getUuid(), buildParams.getOsUser()); + allocNanos = System.nanoTime() - tSpawnStart; + if (allocatedResources == null) { + // No free port. This is the port pool being exhausted, NOT a client that failed to + // start -- the two look identical from the browser and must not be conflated. + LOG.log(Level.WARNING, + "[SPAWN] ALLOC_FAIL osUser=%s | alloc=%dms | no free port in the pool", + buildParams.getOsUser(), + Long.valueOf(allocNanos / 1_000_000L)); return SpawnError.NO_AVAILABLE_PORTS_ERR_CODE.getCode(); } - + ((WebClientBuilder) getBuilder()).getParams().updateOptions(allocatedResources); } + long tFork = System.nanoTime(); Process shell = getBuilder().localStart(environmentMap); + forkAtNanos = System.nanoTime(); + long forkNanos = forkAtNanos - tFork; isSpawnerRunning.set(true); CentralLoggerServer.logSpawnerStream(shell.getErrorStream(), isSpawnerRunning); @@ -374,14 +541,7 @@ Future waitClientFuture = GENERIC_EXECUTOR.submit(() -> { // wait for notification on successful client start - try - { - clientStartupCountDown - .await(CLIENT_STARTUP_TIMEOUT_SEC, TimeUnit.SECONDS); - } - catch (InterruptedException ignored) - { - } + awaitClientStartup("spawnLocal"); }); Integer exitCode = null; @@ -395,15 +555,36 @@ LOG.log(Level.FINE, "Waiting for the exit code of the local spawn was unsuccessful.", e); } + long waitNanos = System.nanoTime() - forkAtNanos; + if (!ready) { shell.destroy(); } - + if (exitCode == null) { exitCode = SpawnError.GENERIC.getCode(); } + + // The verdict line. exit==0 with ready==false means the spawner did its job and the client + // JVM never called back within CLIENT_STARTUP_TIMEOUT_SEC -- i.e. the failure is in client + // start-up, not in spawning, port allocation or account creation. + String outcome = ready ? "READY" + : (exitCode.intValue() == 0 ? "NOT_READY_CLIENT_SILENT" : "SPAWN_ERR"); + LOG.log(Level.WARNING, + "[SPAWN] %s exit=%d ready=%b | alloc=%dms fork=%dms waitClient=%dms total=%dms " + + "| clientStartupTimeoutMs=%d osUser=%s", + outcome, + exitCode, + Boolean.valueOf(ready), + Long.valueOf(allocNanos / 1_000_000L), + Long.valueOf(forkNanos / 1_000_000L), + Long.valueOf(waitNanos / 1_000_000L), + Long.valueOf((System.nanoTime() - tSpawnStart) / 1_000_000L), + Long.valueOf(getClientStartupTimeoutMs()), + getBuildParams().getOsUser()); + return exitCode; } @@ -439,13 +620,7 @@ else if (!ready) { // wait for notification on successful client start - try - { - clientStartupCountDown.await(CLIENT_STARTUP_TIMEOUT_SEC, TimeUnit.SECONDS); - } - catch (InterruptedException ignored) - { - } + awaitClientStartup("spawnRemote"); } return exitCode; } @@ -459,6 +634,17 @@ @Override public void clientIsReady(String data) { + long afterForkMs = forkAtNanos > 0L + ? (System.nanoTime() - forkAtNanos) / 1_000_000L + : -1L; + + // The exact moment the client became usable. Paired with [SPAWN], this separates "client + // was slow but arrived" from "client never arrived". + LOG.log(Level.WARNING, + "[SPAWN-CB] client called back afterForkMs=%d osUser=%s", + Long.valueOf(afterForkMs), + getBuildParams().getOsUser()); + this.ready = true; clientStartupCountDown.countDown(); } === modified file 'src/com/goldencode/p2j/main/WebClientsManager.java' --- old/src/com/goldencode/p2j/main/WebClientsManager.java +++ new/src/com/goldencode/p2j/main/WebClientsManager.java @@ -481,7 +481,10 @@ } List testedWebClientConfigs = new LinkedList(); - + + long tAlloc = System.nanoTime(); + int probes = 0; + int testPort = getTestPort(webClientConfig); InetAddress hostAddr = hostsManager.getResolvedHost(host); try @@ -490,8 +493,10 @@ { try { + probes++; if (!Utils.isEndPointAvailable(hostAddr, testPort, connectionTestTimeout)) { + // Nothing is listening: the port is free and this is the one we will use. break; } } @@ -532,10 +537,22 @@ if (webClientConfig == null) { - LOG.log(Level.WARNING, "All available ports are allocated - no config found."); + LOG.log(Level.WARNING, + "[SPAWN-ALLOC] FAIL no free port after %d probe(s) in %dms (pool exhausted)", + Integer.valueOf(probes), + Long.valueOf((System.nanoTime() - tAlloc) / 1_000_000L)); return null; } + + // Port secured. `discarded` counts ports found already in use; a large value means the + // pool is filling up with clients that were allocated a port and never released it. + LOG.log(Level.WARNING, + "[SPAWN-ALLOC] port=%d probed=%d discarded=%d took=%dms", + Integer.valueOf(webClientConfig.getPort()), + Integer.valueOf(probes), + Integer.valueOf(testedWebClientConfigs.size()), + Long.valueOf((System.nanoTime() - tAlloc) / 1_000_000L)); String webRoot = null; if (viaProxyServer) @@ -561,7 +578,10 @@ } }; cleaners.put(uuid, cleaner); - cleanerTimer.schedule(cleaner, ClientSpawner.CLIENT_STARTUP_TIMEOUT_SEC * 1000); + // Must track the CONFIGURED timeout, not the built-in default: a raised startup timeout + // with a fixed 30s cleaner would free the port out from under a client that is still + // booting, and the next allocation would hand the same port to someone else. + cleanerTimer.schedule(cleaner, ClientSpawner.getClientStartupTimeoutMs()); return webClientConfig; } === modified file 'src/com/goldencode/p2j/main/WebDriverHandler.java' --- old/src/com/goldencode/p2j/main/WebDriverHandler.java +++ new/src/com/goldencode/p2j/main/WebDriverHandler.java @@ -535,6 +535,18 @@ public static Map filterOutValidLoginParams(Fields fields) { Map validatedParams = new HashMap<>(); + + if (fields == null) + { + // The caller passes CLIENT_UUID_LOGIN_PARAMS.remove(uuid), which is null when the entry + // was never registered or has already been consumed -- e.g. a client calling back after + // its spawn was abandoned. This used to NPE and kill the session; report it and carry + // on with no login params, which is what an unregistered client should get anyway. + LOG.log(Level.WARNING, + "[SPAWN-CB] login params missing for a client callback (spawn abandoned, or a " + + "duplicate callback for the same uuid) -- continuing with none"); + return validatedParams; + } for (String expectedParamName : expectedLoginParamNameRegex.keySet()) { @@ -1065,11 +1077,19 @@ CLIENT_UUID_DEVICEID_PAIRS.put(clientUuid, spawnParameters.getDeviceId()); CLIENT_UUID_LOGIN_PARAMS.put(clientUuid, requestParameters.getParamMap()); + long tReq = System.nanoTime(); int exitCode = spawner.spawn(webClientsManager, new String[] { requestParameters.getForwardedHost(), requestParameters.getForwardedProto(), requestParameters.getExternalUserIp()}); + + // Outer boundary: what the browser's POST /gui actually cost and how it ended. + LOG.log(Level.WARNING, + "[SPAWN-REQ] uuid=%s exit=%d totalMs=%d", + clientUuid, + Integer.valueOf(exitCode), + Long.valueOf((System.nanoTime() - tReq) / 1_000_000L)); if (exitCode == 0) { === modified file 'src/com/goldencode/p2j/util/ConfigItem.java' --- old/src/com/goldencode/p2j/util/ConfigItem.java +++ new/src/com/goldencode/p2j/util/ConfigItem.java @@ -310,8 +310,26 @@ public static final ConfigItem HOST = new ConfigItem<>(String.class, "host", "client", "web", "host", Type.WEB_CLIENT); - public static final ConfigItem POLL_TIMEOUT = + public static final ConfigItem POLL_TIMEOUT = new ConfigItem<>(Integer.class, "pollTimeout", "", "", "", Type.WEB_CLIENT); + + /** + * MILLISECONDS to wait for a freshly spawned web client to report itself ready, before the + * server gives up and destroys it. Absent from the directory, {@code ClientSpawner}'s built-in + * default applies. + *

+ * Milliseconds, for consistency with the sibling nodes in this section ({@link #POLL_TIMEOUT}, + * {@link #CONNECTION_TEST_TIMEOUT}). A value that looks like seconds is reported as a probable + * unit mistake rather than silently applied -- see + * ClientSpawner.getClientStartupTimeoutMs(). + *

+ * This bounds CLIENT BOOT, which under load has a long tail: a value below the tail destroys + * healthy clients mid-start-up and makes the caller retry, which adds load and lengthens the + * tail further. See ClientSpawner.getClientStartupTimeoutSec(). + *

+ */ + public static final ConfigItem CLIENT_STARTUP_TIMEOUT = + new ConfigItem<>(Integer.class, "clientStartupTimeout", "", "", "", Type.WEB_CLIENT); public static final ConfigItem CONNECTION_TEST_TIMEOUT = new ConfigItem<>(Integer.class, "connectionTestTimeout", "", "", "", Type.WEB_CLIENT);