Filip/playwright
1
0
Fork 0
Code Issues Pull requests Actions Packages Projects Releases Wiki Activity

fix(tracing): only access tracing state on the API calls, not inside trace operations (#24212)

Browse source 
References #23387.
This commit is contained in:
Dmitry Gozman 2023-07-14 06:19:54 -07:00 committed by GitHub
parent 5d799606c3
commit 98f3ca05b9
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
2 changed files with 82 additions and 85 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

165
packages/playwright-core/src/server/trace/recorder/tracing.ts
View file

@ -74,6 +74,7 @@ export class Tracing extends SdkObject implements InstrumentationListener, Snaps
private _screencastListeners: RegisteredListener[] = []; private _screencastListeners: RegisteredListener[] = [];
private _eventListeners: RegisteredListener[] = []; private _eventListeners: RegisteredListener[] = [];
private _context: BrowserContext | APIRequestContext; private _context: BrowserContext | APIRequestContext;
// Note: state should only be touched inside API methods, but not inside trace operations.
private _state: RecordingState | undefined; private _state: RecordingState | undefined;
private _isStopping = false; private _isStopping = false;
private _precreatedTracesDir: string | undefined; private _precreatedTracesDir: string | undefined;
@ -111,25 +112,20 @@ export class Tracing extends SdkObject implements InstrumentationListener, Snaps
} }
} }
resetForReuse() { async resetForReuse() {
await this.stop();
this._snapshotter?.resetForReuse(); this._snapshotter?.resetForReuse();
} }
async start(options: TracerOptions) { async start(options: TracerOptions) {
if (this._isStopping) if (this._isStopping)
throw new Error('Cannot start tracing while stopping'); throw new Error('Cannot start tracing while stopping');
if (this._state)
throw new Error('Tracing has been already started');
// Re-write for testing. // Re-write for testing.
this._contextCreatedEvent.sdkLanguage = this._context.attribution.playwright.options.sdkLanguage; this._contextCreatedEvent.sdkLanguage = this._context.attribution.playwright.options.sdkLanguage;
if (this._state) {
const o = this._state.options;
if (!o.screenshots !== !options.screenshots || !o.snapshots !== !options.snapshots)
throw new Error('Tracing has been already started with different options');
if (options.name && options.name !== this._state.traceName)
await this._changeTraceName(this._state, options.name);
return;
}
// TODO: passing the same name for two contexts makes them write into a single file // TODO: passing the same name for two contexts makes them write into a single file
// and conflict. // and conflict.
const traceName = options.name || createGuid(); const traceName = options.name || createGuid();
@ -150,8 +146,8 @@ export class Tracing extends SdkObject implements InstrumentationListener, Snaps
recording: false, recording: false,
callIds: new Set(), callIds: new Set(),
}; };
const state = this._state; const { resourcesDir, networkFile } = this._state;
this._writeChain = fs.promises.mkdir(state.resourcesDir, { recursive: true }).then(() => fs.promises.writeFile(state.networkFile.file, '')); this._writeChain = fs.promises.mkdir(resourcesDir, { recursive: true }).then(() => fs.promises.writeFile(networkFile.file, ''));
if (options.snapshots) if (options.snapshots)
this._harTracer.start(); this._harTracer.start();
} }
@ -165,30 +161,30 @@ export class Tracing extends SdkObject implements InstrumentationListener, Snaps
if (this._isStopping) if (this._isStopping)
throw new Error('Cannot start a trace chunk while stopping'); throw new Error('Cannot start a trace chunk while stopping');
const state = this._state; this._state.recording = true;
state.recording = true; this._state.callIds.clear();
state.callIds.clear();
if (options.name && options.name !== state.traceName) if (options.name && options.name !== this._state.traceName)
this._changeTraceName(state, options.name); this._changeTraceName(this._state, options.name);
else else
this._allocateNewTraceFile(state); this._allocateNewTraceFile(this._state);
const { traceFile } = this._state;
this._appendTraceOperation(async () => { this._appendTraceOperation(async () => {
await mkdirIfNeeded(state.traceFile.file); await mkdirIfNeeded(traceFile.file);
const event: trace.TraceEvent = { ...this._contextCreatedEvent, title: options.title, wallTime: Date.now() }; const event: trace.TraceEvent = { ...this._contextCreatedEvent, title: options.title, wallTime: Date.now() };
await appendEventAndFlushIfNeeded(state.traceFile, event); await appendEventAndFlushIfNeeded(traceFile, event);
}); });
this._context.instrumentation.addListener(this, this._context); this._context.instrumentation.addListener(this, this._context);
this._eventListeners.push( this._eventListeners.push(
eventsHelper.addEventListener(this._context, BrowserContext.Events.Console, this._onConsoleMessage.bind(this)), eventsHelper.addEventListener(this._context, BrowserContext.Events.Console, this._onConsoleMessage.bind(this)),
); );
if (state.options.screenshots) if (this._state.options.screenshots)
this._startScreencast(); this._startScreencast();
if (state.options.snapshots) if (this._state.options.snapshots)
await this._snapshotter?.start(); await this._snapshotter?.start();
return { traceName: state.traceName }; return { traceName: this._state.traceName };
} }
private _startScreencast() { private _startScreencast() {
@ -218,21 +214,22 @@ export class Tracing extends SdkObject implements InstrumentationListener, Snaps
}; };
} }
private async _changeTraceName(state: RecordingState, name: string) { private _changeTraceName(state: RecordingState, name: string) {
await this._appendTraceOperation(async () => { const { traceFile: oldTraceFile, networkFile: oldNetworkFile } = state;
await flushTraceFile(state.traceFile); state.traceName = name;
await flushTraceFile(state.networkFile); state.chunkOrdinal = 0; // Reset ordinal for the new name.
this._allocateNewTraceFile(state);
state.networkFile = {
file: path.join(state.tracesDir, name + '.network'),
buffer: [],
};
const networkFile = state.networkFile;
const oldNetworkFile = state.networkFile; this._appendTraceOperation(async () => {
state.traceName = name; await flushTraceFile(oldTraceFile);
state.chunkOrdinal = 0; // Reset ordinal for the new name. await flushTraceFile(oldNetworkFile);
this._allocateNewTraceFile(state);
state.networkFile = {
file: path.join(state.tracesDir, name + '.network'),
buffer: [],
};
// Network file survives across chunks, so make a copy with the new name. // Network file survives across chunks, so make a copy with the new name.
await fs.promises.copyFile(oldNetworkFile.file, state.networkFile.file); await fs.promises.copyFile(oldNetworkFile.file, networkFile.file);
}); });
} }
@ -278,59 +275,67 @@ export class Tracing extends SdkObject implements InstrumentationListener, Snaps
return {}; return {};
} }
const state = this._state!;
this._context.instrumentation.removeListener(this); this._context.instrumentation.removeListener(this);
eventsHelper.removeEventListeners(this._eventListeners); eventsHelper.removeEventListeners(this._eventListeners);
if (this._state?.options.screenshots) if (this._state.options.screenshots)
this._stopScreencast(); this._stopScreencast();
if (state.options.snapshots) if (this._state.options.snapshots)
await this._snapshotter?.stop(); await this._snapshotter?.stop();
// Network file survives across chunks, make a snapshot before returning the resulting entries.
// We should pick a name starting with "traceName" and ending with .network.
// Something like <traceName>someSuffixHere.network.
// However, this name must not clash with any other "traceName".network in the same tracesDir.
// We can use <traceName>-<guid>.network, but "-pwnetcopy-0" suffix is more readable
// and makes it easier to debug future issues.
const newNetworkFile = path.join(this._state.tracesDir, this._state.traceName + `-pwnetcopy-${this._state.chunkOrdinal}.network`);
const oldNetworkFile = this._state.networkFile;
const traceFile = this._state.traceFile;
const entries: NameValue[] = [];
entries.push({ name: 'trace.trace', value: traceFile.file });
entries.push({ name: 'trace.network', value: newNetworkFile });
for (const sha1 of new Set([...this._state.traceSha1s, ...this._state.networkSha1s]))
entries.push({ name: path.join('resources', sha1), value: path.join(this._state.resourcesDir, sha1) });
// Only reset trace sha1s, network resources are preserved between chunks.
this._state.traceSha1s = new Set();
// Chain the export operation against write operations, // Chain the export operation against write operations,
// so that neither trace files nor sha1s change during the export. // so that neither trace files nor resources change during the export.
return await this._appendTraceOperation(async () => { let result: { artifact?: Artifact, entries?: NameValue[] } = {};
this._appendTraceOperation(async () => {
if (params.mode === 'discard') if (params.mode === 'discard')
return {}; return;
await flushTraceFile(state.traceFile); await flushTraceFile(traceFile);
await flushTraceFile(state.networkFile); await flushTraceFile(oldNetworkFile);
await fs.promises.copyFile(oldNetworkFile.file, newNetworkFile);
// Network file survives across chunks, make a snapshot before returning the resulting entries.
// We should pick a name starting with "traceName" and ending with .network.
// Something like <traceName>someSuffixHere.network.
// However, this name must not clash with any other "traceName".network in the same tracesDir.
// We can use <traceName>-<guid>.network, but "-pwnetcopy-0" suffix is more readable
// and makes it easier to debug future issues.
const networkFile = path.join(state.tracesDir, state.traceName + `-pwnetcopy-${state.chunkOrdinal}.network`);
await fs.promises.copyFile(state.networkFile.file, networkFile);
const entries: NameValue[] = [];
entries.push({ name: 'trace.trace', value: state.traceFile.file });
entries.push({ name: 'trace.network', value: networkFile });
for (const sha1 of new Set([...state.traceSha1s, ...state.networkSha1s]))
entries.push({ name: path.join('resources', sha1), value: path.join(state.resourcesDir, sha1) });
if (params.mode === 'entries') if (params.mode === 'entries')
return { entries }; result = { entries };
const artifact = await this._exportZip(entries, state).catch(() => undefined); else
return { artifact }; result = { artifact: await this._exportZip(entries, traceFile.file + '.zip').catch(() => undefined) };
}).finally(() => { });
// Only reset trace sha1s, network resources are preserved between chunks.
state.traceSha1s = new Set(); try {
await this._writeChain;
return result;
} finally {
this._isStopping = false; this._isStopping = false;
state.recording = false; if (this._state)
}) || { }; this._state.recording = false;
}
} }
private _exportZip(entries: NameValue[], state: RecordingState): Promise<Artifact | undefined> { private _exportZip(entries: NameValue[], zipFileName: string): Promise<Artifact | undefined> {
const zipFile = new yazl.ZipFile(); const zipFile = new yazl.ZipFile();
const result = new ManualPromise<Artifact | undefined>(); const result = new ManualPromise<Artifact | undefined>();
(zipFile as any as EventEmitter).on('error', error => result.reject(error)); (zipFile as any as EventEmitter).on('error', error => result.reject(error));
for (const entry of entries) for (const entry of entries)
zipFile.addFile(entry.value, entry.name); zipFile.addFile(entry.value, entry.name);
zipFile.end(); zipFile.end();
const zipFileName = state.traceFile.file + '.zip';
zipFile.outputStream.pipe(fs.createWriteStream(zipFileName)).on('close', () => { zipFile.outputStream.pipe(fs.createWriteStream(zipFileName)).on('close', () => {
const artifact = new Artifact(this._context, zipFileName); const artifact = new Artifact(this._context, zipFileName);
artifact.reportFinished(); artifact.reportFinished();
@ -404,9 +409,10 @@ export class Tracing extends SdkObject implements InstrumentationListener, Snaps
onEntryFinished(entry: har.Entry) { onEntryFinished(entry: har.Entry) {
const event: trace.ResourceSnapshotTraceEvent = { type: 'resource-snapshot', snapshot: entry }; const event: trace.ResourceSnapshotTraceEvent = { type: 'resource-snapshot', snapshot: entry };
const visited = visitTraceEvent(event, this._state!.networkSha1s);
const { networkFile } = this._state!;
this._appendTraceOperation(async () => { this._appendTraceOperation(async () => {
const visited = visitTraceEvent(event, this._state!.networkSha1s); await appendEventAndFlushIfNeeded(networkFile, visited);
await appendEventAndFlushIfNeeded(this._state!.networkFile, visited);
}); });
} }
@ -469,9 +475,10 @@ export class Tracing extends SdkObject implements InstrumentationListener, Snaps
} }
private _appendTraceEvent(event: trace.TraceEvent) { private _appendTraceEvent(event: trace.TraceEvent) {
const visited = visitTraceEvent(event, this._state!.traceSha1s);
const { traceFile } = this._state!;
this._appendTraceOperation(async () => { this._appendTraceOperation(async () => {
const visited = visitTraceEvent(event, this._state!.traceSha1s); await appendEventAndFlushIfNeeded(traceFile, visited);
await appendEventAndFlushIfNeeded(this._state!.traceFile, visited);
}); });
} }
@ -488,25 +495,15 @@ export class Tracing extends SdkObject implements InstrumentationListener, Snaps
}); });
} }
private async _appendTraceOperation<T>(cb: () => Promise<T>): Promise<T | undefined> { private _appendTraceOperation(cb: () => Promise<unknown>): void {
// This method serializes all writes to the trace. // This method serializes all writes to the trace.
let error: Error | undefined;
let result: T | undefined;
this._writeChain = this._writeChain.then(async () => { this._writeChain = this._writeChain.then(async () => {
// This check is here because closing the browser removes the tracesDir and tracing // This check is here because closing the browser removes the tracesDir and tracing
// dies trying to archive. // dies trying to archive.
if (this._context instanceof BrowserContext && !this._context._browser.isConnected()) if (this._context instanceof BrowserContext && !this._context._browser.isConnected())
return; return;
try { await cb();
result = await cb();
} catch (e) {
error = e;
}
}); });
await this._writeChain;
if (error)
throw error;
return result;
} }
} }

2
tests/library/tracing.spec.ts
View file

@ -414,7 +414,7 @@ test('should include interrupted actions', async ({ context, page, server }, tes
test('should throw when starting with different options', async ({ context }) => { test('should throw when starting with different options', async ({ context }) => {
await context.tracing.start({ screenshots: true, snapshots: true }); await context.tracing.start({ screenshots: true, snapshots: true });
const error = await context.tracing.start({ screenshots: false, snapshots: false }).catch(e => e); const error = await context.tracing.start({ screenshots: false, snapshots: false }).catch(e => e);
expect(error.message).toContain('Tracing has been already started with different options'); expect(error.message).toContain('Tracing has been already started');
}); });
test('should throw when stopping without start', async ({ context }, testInfo) => { test('should throw when stopping without start', async ({ context }, testInfo) => {