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.

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

fix incompatibility with express-async-errors #555

Merged
merged 1 commit into from
May 6, 2019

Conversation

rochdev
Copy link
Member

@rochdev rochdev commented May 2, 2019

This PR fixes an incompatibility with express-async-errors. The module defines a getter/setter on Layer.prototype.handle where it wraps the layer handler when it gets set. The problem is that we overwrite the handler in the express instrumentation, but after it's already been wrapped by the module. This means that we wrap their wrapper, and then reassign layer.handle which causes the setter to wrap our wrapper. This end up with a situation where we then have asyncWrapper -> ddWrapper -> asyncWrapper -> handler which results in unexpected behaviors.

Since express-async-errors adds a __handle property to the layer, we can patch that instead without triggering the setter.

The behavior is different depending on the version of Node and in some cases internal to Express, so I was unable to write a proper unit test for this.

Fixes #548

@rochdev rochdev added bug Something isn't working integrations labels May 2, 2019
@rochdev rochdev added this to the 0.11.1 milestone May 2, 2019
@rochdev rochdev requested a review from brettlangdon May 2, 2019 23:19
@rochdev rochdev merged commit ba9be7c into master May 6, 2019
@rochdev rochdev deleted the express-async-errors branch May 6, 2019 17:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working integrations
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Express Error Handling Overwriting Other Patch
2 participants