Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

core(target-manager): warn on errors while attaching to workers #15740

Merged
merged 3 commits into from
Jan 18, 2024
Merged

core(target-manager): warn on errors while attaching to workers #15740

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
95580cc
core(target-manager): warn on errors with worker targets
adamraine Jan 10, 2024
d69dc34
test
adamraine Jan 10, 2024
2fd09e1
Merge branch 'main' into warn-worker-target
adamraine Jan 12, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 10 additions & 1 deletion core/gather/driver/target-manager.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -125,9 +125,11 @@
async _onSessionAttached(cdpSession) {
const newSession = new ProtocolSession(cdpSession);

let targetType;

try {
const {targetInfo} = await newSession.sendCommand('Target.getTargetInfo');
const targetType = targetInfo.type;
targetType = targetInfo.type;

// TODO: should detach from target in this case?
// See pptr: https://github.com/puppeteer/puppeteer/blob/733cbecf487c71483bee8350e37030edb24bc021/src/common/Page.ts#L495-L526
Expand Down Expand Up @@ -168,6 +170,13 @@
// Sometimes targets can be closed before we even have a chance to listen to their network activity.
if (/Target closed/.test(err.message)) return;

// Worker targets can be a bit fickle and we only enable them for diagnostic purposes.
// We shouldn't throw a fatal error if there were issues attaching to them.
if (targetType === 'worker') {
log.warn('target-manager', `Issue attaching to worker target: ${err}`);
return;
}

Check warning on line 179 in core/gather/driver/target-manager.js

View check run for this annotation

Codecov / codecov/patch

core/gather/driver/target-manager.js#L173-L179 

Added lines #L173 - L179 were not covered by tests
throw err;
} finally {
// Resume the target if it was paused, but if it's unnecessary, we don't care about the error.
Expand Down
17 changes: 17 additions & 0 deletions core/test/gather/driver/target-manager-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,6 +108,23 @@ describe('TargetManager', () => {
expect(sendMock.findAllInvocations('Runtime.runIfWaitingForDebugger')).toHaveLength(4);
});

it('should ignore errors while attaching to worker targets', async () => {
targetInfo.type = 'worker';
sendMock
.mockResponse('Target.getTargetInfo', {targetInfo})
.mockResponse('Network.enable', () => {
throw new Error('Cannot use Network.enable');
})
.mockResponse('Target.setAutoAttach');
await targetManager.enable();

const invocations = sendMock.findAllInvocations('Target.setAutoAttach');
expect(invocations).toHaveLength(0);

// Should still be resumed.
expect(sendMock.findAllInvocations('Runtime.runIfWaitingForDebugger')).toHaveLength(1);
});

it('should ignore targets that are not frames or web workers', async () => {
targetInfo.type = 'service_worker';
sendMock
Expand Down
Loading