Skip to content

fix(node): Ensure that self._handler exists before calling it in LinkedErrors #5497

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

Merged
merged 1 commit into from
Aug 1, 2022
Merged

fix(node): Ensure that self._handler exists before calling it in LinkedErrors #5497

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
2 changes: 1 addition & 1 deletion packages/node/src/integrations/linkederrors.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ export class LinkedErrors implements Integration {
const hub = getCurrentHub();
const self = hub.getIntegration(LinkedErrors);
const client = hub.getClient<NodeClient>();
if (client && self) {
if (client && self && self._handler && typeof self._handler === 'function') {
Copy link

@shellscape shellscape Sep 18, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Lms24 @AbhiPrasad this should have included a check that event was truthy and an object and if not, returned, since calling _handler with an invalid event object does the same. see: #5622 (comment)

The package should also warn to console when something. like this happens, or at least output to a debug channel that can be enabled.

If the package had some better internal error handling and better debug logging, I could have narrowed this down further. Alas.

await self._handler(client.getOptions().stackParser, event, hint);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand how self._handler could ever error, but we gotta do this to unblock

sad cat thumbs up

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand how this happens either but this should fix it for now. Will try to investigate this more on Monday. Since we're cutting a patch on Monday, I'll leave this open till then

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I had to guess it's due to multiple @sentry/nodes being installed - maybe through transitive deps?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that a version conflict might cause this. Apart from that I don't see how this method would not exist while the class instance obviously does exist.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Merging this for now as I think the check doesn't hurt us

Copy link

@public public Aug 1, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I checked out package-lock.json when we had 7.8.0 installed and can't see any references to other versions of sentry/node in there. Happy to share the full file if you think it would be helpful.

}
return event;
Expand Down