chore: remove legacy timeouts support in Progress (microsoft#36744)

Author: dgozman

.github/workflows/tests_others.yml

@@ -134,27 +134,6 @@ jobs:
       env:
         PW_CLOCK: ${{ matrix.clock }}
 
-  test_legacy_progress_timeouts:
-    name: legacy progress timeouts
-    environment: ${{ github.event_name == 'push' && 'allow-uploading-flakiness-results' || null }}
-    permissions:
-      id-token: write   # This is required for OIDC login (azure/login) to succeed
-      contents: read    # This is required for actions/checkout to succeed
-    runs-on: ubuntu-22.04
-    steps:
-    - uses: actions/checkout@v4
-    - uses: ./.github/actions/run-test
-      with:
-        node-version: 20
-        browsers-to-install: chromium
-        command: npm run test -- --project=chromium-*
-        bot-name: "legacy-progress-timeouts-linux"
-        flakiness-client-id: ${{ secrets.AZURE_FLAKINESS_DASHBOARD_CLIENT_ID }}
-        flakiness-tenant-id: ${{ secrets.AZURE_FLAKINESS_DASHBOARD_TENANT_ID }}
-        flakiness-subscription-id: ${{ secrets.AZURE_FLAKINESS_DASHBOARD_SUBSCRIPTION_ID }}
-      env:
-        PLAYWRIGHT_LEGACY_TIMEOUTS: 1
-
   test_electron:
     name: Electron - ${{ matrix.os }}
     environment: ${{ github.event_name == 'push' && 'allow-uploading-flakiness-results' || null }}

packages/playwright-core/src/server/dispatchers/frameDispatcher.ts

@@ -266,11 +266,7 @@ export class FrameDispatcher extends Dispatcher<Frame, channels.FrameChannel, Br
     let expectedValue = params.expectedValue ? parseArgument(params.expectedValue) : undefined;
     if (params.expression === 'to.match.aria' && expectedValue)
       expectedValue = parseAriaSnapshotUnsafe(yaml, expectedValue);
-    const result = await this._frame.expect(progress, params.selector, { ...params, expectedValue }, params.timeout, result => {
-      if (result.received !== undefined)
-        result.received = serializeResult(result.received);
-      return result;
-    });
+    const result = await this._frame.expect(progress, params.selector, { ...params, expectedValue }, params.timeout);
     if (result.received !== undefined)
       result.received = serializeResult(result.received);
     return result;

packages/playwright-core/src/server/frames.ts

@@ -1345,35 +1345,14 @@ export class Frame extends SdkObject {
     return await this._retryWithProgressIfNotConnected(progress, selector, true /* strict */, true /* performActionPreChecks */, handle => progress.race(handle.ariaSnapshot(options)));
   }
 
-  // TODO: remove errorResultHandler once legacy progress is removed.
-  async expect(progress: Progress, selector: string | undefined, options: FrameExpectParams, timeout?: number, errorResultHandler?: (result: ExpectResult) => ExpectResult): Promise<ExpectResult> {
+  async expect(progress: Progress, selector: string | undefined, options: FrameExpectParams, timeout?: number): Promise<ExpectResult> {
     progress.log(`${renderTitleForCall(progress.metadata)}${timeout ? ` with timeout ${timeout}ms` : ''}`);
-    const result = await this._expectImpl(progress, selector, options, errorResultHandler);
-    return result;
-  }
-
-  private async _expectImpl(progress: Progress, selector: string | undefined, options: FrameExpectParams, errorResultHandler?: (result: ExpectResult) => ExpectResult): Promise<ExpectResult> {
     const lastIntermediateResult: { received?: any, isSet: boolean } = { isSet: false };
     const fixupMetadataError = (result: ExpectResult) => {
       // Library mode special case for the expect errors which are return values, not exceptions.
       if (result.matches === options.isNot)
         progress.metadata.error = { error: { name: 'Expect', message: 'Expect failed' } };
     };
-    const handleError = (e: any, isProgressError: boolean) => {
-      // Q: Why not throw upon isNonRetriableError(e) as in other places?
-      // A: We want user to receive a friendly message containing the last intermediate result.
-      if (js.isJavaScriptErrorInEvaluate(e) || isInvalidSelectorError(e))
-        throw e;
-      const result: ExpectResult = { matches: options.isNot, log: compressCallLog(progress.metadata.log) };
-      if (lastIntermediateResult.isSet)
-        result.received = lastIntermediateResult.received;
-      if (e instanceof TimeoutError)
-        result.timedOut = true;
-      fixupMetadataError(result);
-      return (isProgressError && errorResultHandler) ? errorResultHandler(result) : result;
-    };
-    progress.legacySetErrorHandler(e => handleError(e, true));
-
     try {
       // Step 1: perform locator handlers checkpoint with a specified timeout.
       if (selector)
@@ -1383,7 +1362,6 @@ export class Frame extends SdkObject {
       // Step 2: perform one-shot expect check without a timeout.
       // Supports the case of `expect(locator).toBeVisible({ timeout: 1 })`
       // that should succeed when the locator is already visible.
-      progress.legacyDisableTimeout();
       try {
         const resultOneShot = await this._expectInternal(progress, selector, options, lastIntermediateResult, true);
         if (resultOneShot.matches !== options.isNot)
@@ -1393,7 +1371,6 @@ export class Frame extends SdkObject {
           throw e;
         // Ignore any other errors from one-shot, we'll handle them during retries.
       }
-      progress.legacyEnableTimeout();
 
       // Step 3: auto-retry expect with increasing timeouts. Bounded by the total remaining time.
       const result = await this.retryWithProgressAndTimeouts(progress, [100, 250, 500, 1000], async continuePolling => {
@@ -1410,7 +1387,17 @@ export class Frame extends SdkObject {
       fixupMetadataError(result);
       return result;
     } catch (e) {
-      return handleError(e, false);
+      // Q: Why not throw upon isNonRetriableError(e) as in other places?
+      // A: We want user to receive a friendly message containing the last intermediate result.
+      if (js.isJavaScriptErrorInEvaluate(e) || isInvalidSelectorError(e))
+        throw e;
+      const result: ExpectResult = { matches: options.isNot, log: compressCallLog(progress.metadata.log) };
+      if (lastIntermediateResult.isSet)
+        result.received = lastIntermediateResult.received;
+      if (e instanceof TimeoutError)
+        result.timedOut = true;
+      fixupMetadataError(result);
+      return result;
     }
   }
 

packages/playwright-core/src/server/page.ts

@@ -632,22 +632,6 @@ export class Page extends SdkObject {
         intermediateResult = { errorMessage: comparatorResult.errorMessage, diff: comparatorResult.diff, actual, previous };
       return false;
     };
-    const handleError = (e: any) => {
-      // Q: Why not throw upon isNonRetriableError(e) as in other places?
-      // A: We want user to receive a friendly diff between actual and expected/previous.
-      if (js.isJavaScriptErrorInEvaluate(e) || isInvalidSelectorError(e))
-        throw e;
-      let errorMessage = e.message;
-      if (e instanceof TimeoutError && intermediateResult?.previous)
-        errorMessage = `Failed to take two consecutive stable screenshots.`;
-      return {
-        log: compressCallLog(e.message ? [...progress.metadata.log, e.message] : progress.metadata.log),
-        ...intermediateResult,
-        errorMessage,
-        timedOut: (e instanceof TimeoutError),
-      };
-    };
-    progress.legacySetErrorHandler(handleError);
 
     try {
       let actual: Buffer | undefined;
@@ -700,7 +684,19 @@ export class Page extends SdkObject {
       }
       throw new Error(intermediateResult!.errorMessage);
     } catch (e) {
-      return handleError(e);
+      // Q: Why not throw upon isNonRetriableError(e) as in other places?
+      // A: We want user to receive a friendly diff between actual and expected/previous.
+      if (js.isJavaScriptErrorInEvaluate(e) || isInvalidSelectorError(e))
+        throw e;
+      let errorMessage = e.message;
+      if (e instanceof TimeoutError && intermediateResult?.previous)
+        errorMessage = `Failed to take two consecutive stable screenshots.`;
+      return {
+        log: compressCallLog(e.message ? [...progress.metadata.log, e.message] : progress.metadata.log),
+        ...intermediateResult,
+        errorMessage,
+        timedOut: (e instanceof TimeoutError),
+      };
     }
   }
 

packages/playwright-core/src/server/progress.ts

@@ -15,11 +15,11 @@
  */
 
 import { TimeoutError } from './errors';
-import { assert, monotonicTime } from '../utils';
+import { assert } from '../utils';
 import { ManualPromise } from '../utils/isomorphic/manualPromise';
 
 import type { Progress } from '@protocol/progress';
-import type { CallMetadata, Instrumentation, SdkObject } from './instrumentation';
+import type { CallMetadata, SdkObject } from './instrumentation';
 import type { LogName } from './utils/debugLogger';
 
 export type { Progress } from '@protocol/progress';
@@ -31,24 +31,15 @@ export class ProgressController {
   // Cleanups to be run only in the case of abort.
   private _cleanups: ((error: Error | undefined) => any)[] = [];
 
-  // Lenient mode races against the timeout. This guarantees that timeout is respected,
-  // but may have some work being done after the timeout due to parallel control flow.
-  //
-  // Strict mode aborts the progress and requires the code to react to it. This way,
-  // progress only finishes after the inner callback exits, guaranteeing no work after the timeout.
-  private _strictMode = false;
-
   private _logName: LogName;
   private _state: 'before' | 'running' | { error: Error } | 'finished' = 'before';
+  private _sdkObject: SdkObject;
+
   readonly metadata: CallMetadata;
-  readonly instrumentation: Instrumentation;
-  readonly sdkObject: SdkObject;
 
   constructor(metadata: CallMetadata, sdkObject: SdkObject) {
-    this._strictMode = !process.env.PLAYWRIGHT_LEGACY_TIMEOUTS;
     this.metadata = metadata;
-    this.sdkObject = sdkObject;
-    this.instrumentation = sdkObject.instrumentation;
+    this._sdkObject = sdkObject;
     this._logName = sdkObject.logName || 'api';
     this._forceAbortPromise.catch(e => null);  // Prevent unhandled promise rejection.
   }
@@ -63,54 +54,24 @@ export class ProgressController {
       this._state = { error };
       this._forceAbortPromise.reject(error);
     }
-    if (this._strictMode)
-      await this._donePromise;
+    await this._donePromise;
   }
 
   async run<T>(task: (progress: Progress) => Promise<T>, timeout?: number): Promise<T> {
     assert(this._state === 'before');
     this._state = 'running';
-    let customErrorHandler: ((error: Error) => any) | undefined;
-
-    const deadline = timeout ? Math.min(monotonicTime() + timeout, 2147483647) : 0; // 2^31-1 safe setTimeout in Node.
-    const timeoutError = new TimeoutError(`Timeout ${timeout}ms exceeded.`);
-
-    let timer: NodeJS.Timeout | undefined;
-    const startTimer = () => {
-      if (!deadline)
-        return;
-      const onTimeout = () => {
-        if (this._state === 'running') {
-          (timeoutError as any)[kAbortErrorSymbol] = true;
-          this._state = { error: timeoutError };
-          this._forceAbortPromise.reject(timeoutError);
-        }
-      };
-      const remaining = deadline - monotonicTime();
-      if (remaining <= 0)
-        onTimeout();
-      else
-        timer = setTimeout(onTimeout, remaining);
-    };
 
     const progress: Progress = {
       log: message => {
         if (this._state === 'running')
           this.metadata.log.push(message);
         // Note: we might be sending logs after progress has finished, for example browser logs.
-        this.instrumentation.onCallLog(this.sdkObject, this.metadata, this._logName, message);
+        this._sdkObject.instrumentation.onCallLog(this._sdkObject, this.metadata, this._logName, message);
       },
       cleanupWhenAborted: (cleanup: (error: Error | undefined) => any) => {
-        if (this._strictMode) {
-          if (this._state !== 'running')
-            throw new Error('Internal error: cannot register cleanup after operation has finished.');
-          this._cleanups.push(cleanup);
-          return;
-        }
-        if (this._state === 'running')
-          this._cleanups.push(cleanup);
-        else
-          runCleanup(typeof this._state === 'object' ? this._state.error : undefined, cleanup);
+        if (this._state !== 'running')
+          throw new Error('Internal error: cannot register cleanup after operation has finished.');
+        this._cleanups.push(cleanup);
       },
       metadata: this.metadata,
       race: <T>(promise: Promise<T> | Promise<T>[]) => {
@@ -131,35 +92,27 @@ export class ProgressController {
         const promise = new Promise<void>(f => timer = setTimeout(f, timeout));
         return progress.race(promise).finally(() => clearTimeout(timer));
       },
-      legacyDisableTimeout: () => {
-        if (this._strictMode)
-          return;
-        clearTimeout(timer);
-      },
-      legacyEnableTimeout: () => {
-        if (this._strictMode)
-          return;
-        startTimer();
-      },
-      legacySetErrorHandler: (handler: (error: Error) => any) => {
-        if (this._strictMode)
-          return;
-        customErrorHandler = handler;
-      },
     };
 
-    startTimer();
+    let timer: NodeJS.Timeout | undefined;
+    if (timeout) {
+      const timeoutError = new TimeoutError(`Timeout ${timeout}ms exceeded.`);
+      timer = setTimeout(() => {
+        if (this._state === 'running') {
+          (timeoutError as any)[kAbortErrorSymbol] = true;
+          this._state = { error: timeoutError };
+          this._forceAbortPromise.reject(timeoutError);
+        }
+      }, timeout);
+    }
 
     try {
-      const promise = task(progress);
-      const result = this._strictMode ? await promise : await Promise.race([promise, this._forceAbortPromise]);
+      const result = await task(progress);
       this._state = 'finished';
       return result;
     } catch (error) {
       this._state = { error };
       await Promise.all(this._cleanups.splice(0).map(cleanup => runCleanup(error, cleanup)));
-      if (customErrorHandler)
-        return customErrorHandler(error);
       throw error;
     } finally {
       clearTimeout(timer);

packages/protocol/src/progress.d.ts

@@ -38,9 +38,4 @@ export interface Progress {
   raceWithCleanup<T>(promise: Promise<T>, cleanup: (result: T) => any): Promise<T>;
   wait(timeout: number): Promise<void>;
   metadata: CallMetadata;
-
-  // Legacy lenient mode api only. To be removed.
-  legacyDisableTimeout(): void;
-  legacyEnableTimeout(): void;
-  legacySetErrorHandler(handler: (error: Error) => void): void;
 }