From a83646684a5d47c147a0e85f38a966ab6e2b9de8 Mon Sep 17 00:00:00 2001 From: Dmitry Gozman Date: Fri, 4 Jun 2021 14:52:16 -0700 Subject: [PATCH] fix(tracing): error handling (#6888) - Reject when ZipFile signals an error. - Make sure snapshotter does not save trace events after stop(). - Await pending blob writes on stop(). --- src/server/snapshot/snapshotter.ts | 2 +- src/server/trace/recorder/traceSnapshotter.ts | 1 + src/server/trace/recorder/tracing.ts | 40 +++++++++++-------- tests/tracing.spec.ts | 2 +- 4 files changed, 26 insertions(+), 19 deletions(-) diff --git a/src/server/snapshot/snapshotter.ts b/src/server/snapshot/snapshotter.ts index a83eb45c7d..03e88ac667 100644 --- a/src/server/snapshot/snapshotter.ts +++ b/src/server/snapshot/snapshotter.ts @@ -116,7 +116,7 @@ export class Snapshotter { const snapshots = page.frames().map(async frame => { const data = await frame.nonStallingRawEvaluateInExistingMainContext(expression).catch(e => debugLogger.log('error', e)) as SnapshotData; // Something went wrong -> bail out, our snapshots are best-efforty. - if (!data) + if (!data || !this._started) return; const snapshot: FrameSnapshot = { diff --git a/src/server/trace/recorder/traceSnapshotter.ts b/src/server/trace/recorder/traceSnapshotter.ts index 5c1b3ab540..0088d16d62 100644 --- a/src/server/trace/recorder/traceSnapshotter.ts +++ b/src/server/trace/recorder/traceSnapshotter.ts @@ -48,6 +48,7 @@ export class TraceSnapshotter extends EventEmitter implements SnapshotterDelegat async stop(): Promise { await this._snapshotter.stop(); + await this._writeArtifactChain; } async dispose() { diff --git a/src/server/trace/recorder/tracing.ts b/src/server/trace/recorder/tracing.ts index 59884449f7..5b37f9add6 100644 --- a/src/server/trace/recorder/tracing.ts +++ b/src/server/trace/recorder/tracing.ts @@ -17,6 +17,7 @@ import fs from 'fs'; import path from 'path'; import yazl from 'yazl'; +import { EventEmitter } from 'events'; import { calculateSha1, createGuid, mkdirIfNeeded, monotonicTime } from '../../../utils/utils'; import { Artifact } from '../../artifact'; import { BrowserContext } from '../../browserContext'; @@ -43,7 +44,7 @@ export class Tracing implements InstrumentationListener { private _resourcesDir: string; private _sha1s: string[] = []; private _started = false; - private _tracesDir: string | undefined; + private _tracesDir: string; constructor(context: BrowserContext) { this._context = context; @@ -54,8 +55,6 @@ export class Tracing implements InstrumentationListener { async start(options: TracerOptions): Promise { // context + page must be the first events added, this method can't have awaits before them. - if (!this._tracesDir) - throw new Error('Tracing directory is not specified when launching the browser'); if (this._started) throw new Error('Tracing has already been started'); this._started = true; @@ -86,13 +85,13 @@ export class Tracing implements InstrumentationListener { if (!this._started) return; this._started = false; - await this._snapshotter.stop(); this._context.instrumentation.removeListener(this); helper.removeEventListeners(this._eventListeners); for (const { sdkObject, metadata } of this._pendingCalls.values()) await this.onAfterCall(sdkObject, metadata); for (const page of this._context.pages()) page.setScreencastOptions(null); + await this._snapshotter.stop(); // Ensure all writes are finished. await this._appendEventChain; @@ -103,21 +102,25 @@ export class Tracing implements InstrumentationListener { } async export(): Promise { - if (!this._traceFile) - throw new Error('Tracing directory is not specified when launching the browser'); + if (!this._traceFile || this._started) + throw new Error('Must start and stop tracing before exporting'); const zipFile = new yazl.ZipFile(); - zipFile.addFile(this._traceFile, 'trace.trace'); - const zipFileName = this._traceFile + '.zip'; - this._traceFile = undefined; - for (const sha1 of this._sha1s) - zipFile.addFile(path.join(this._resourcesDir!, sha1), path.join('resources', sha1)); - zipFile.end(); - await new Promise(f => { - zipFile.outputStream.pipe(fs.createWriteStream(zipFileName)).on('close', f); + const failedPromise = new Promise((_, reject) => (zipFile as any as EventEmitter).on('error', reject)); + + const succeededPromise = new Promise(async fulfill => { + zipFile.addFile(this._traceFile!, 'trace.trace'); + const zipFileName = this._traceFile! + '.zip'; + for (const sha1 of this._sha1s) + zipFile.addFile(path.join(this._resourcesDir!, sha1), path.join('resources', sha1)); + zipFile.end(); + await new Promise(f => { + zipFile.outputStream.pipe(fs.createWriteStream(zipFileName)).on('close', f); + }); + const artifact = new Artifact(this._context, zipFileName); + artifact.reportFinished(); + fulfill(artifact); }); - const artifact = new Artifact(this._context, zipFileName); - artifact.reportFinished(); - return artifact; + return Promise.race([failedPromise, succeededPromise]); } async _captureSnapshot(name: 'before' | 'after' | 'action' | 'event', sdkObject: SdkObject, metadata: CallMetadata, element?: ElementHandle) { @@ -181,6 +184,9 @@ export class Tracing implements InstrumentationListener { } private _appendTraceEvent(event: any) { + if (!this._started) + return; + const visit = (object: any) => { if (Array.isArray(object)) { object.forEach(visit); diff --git a/tests/tracing.spec.ts b/tests/tracing.spec.ts index 700b3ad309..f97af0df61 100644 --- a/tests/tracing.spec.ts +++ b/tests/tracing.spec.ts @@ -38,7 +38,7 @@ test('should collect trace', async ({ context, page, server, browserName }, test expect(events.some(e => e.type === 'screencast-frame')).toBeTruthy(); }); -test('should collect trace', async ({ context, page, server }, testInfo) => { +test('should not collect snapshots by default', async ({ context, page, server }, testInfo) => { await context.tracing.start(); await page.goto(server.EMPTY_PAGE); await page.setContent('');