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: ensure correct streaming when using mimic-response #429

Merged
merged 7 commits into from
Jul 4, 2018

Conversation

watson
Copy link
Contributor

@watson watson commented Jul 2, 2018

The mimic-response module exposes a single function with the following API:

mimicResponse(fromStream, toStream)

If someone is calling mimicResponse where the fromStream have had its emitter functions bound using bindEmitter(fromStream), the toStream will wrongly bind all its event listeners to the fromStream.

This becomes especially problematic in case the data is transformed either in the toStream or in between fromStream and toStream, eg:

fromStream.pipe(zlib.createGunzip()).pipe(toStream)

In those cases, the data emitted by toStream will actually be the un-transformed data coming from the fromStream.

This commit patches mimic-response so that the event emitters of toStream runs in the expected context.

Fixes #423

The `mimic-response` module exposes a single function with the following
API:

    mimicResponse(fromStream, toStream)

If someone is calling `mimicResponse` where the `fromStream` have had
its emitter functions bound using `bindEmitter(fromStream)`, the
`toStream` will wrongly bind all its event listeners to the
`fromStream`.

This becomes especially problematic in case the data is transformed
either in the `toStream` or in between `fromStream` and `toStream`, eg:

    fromStream.pipe(zlib.createGunzip()).pipe(toStream)

In those cases, the data emitted by `toStream` will actually be the
un-transformed data coming from the `fromStream`.

This commit patches `mimic-response` so that the event emitters of
`toStream` runs in the expected context.

Fixes elastic#423
@watson watson self-assigned this Jul 2, 2018
@watson watson requested a review from Qard July 2, 2018 14:06
@watson
Copy link
Contributor Author

watson commented Jul 2, 2018

Btw, I discovered that this isn't the first time we've had issues with mimic-response. We already added some tests to make sure we were compatible in 4f2b9eb (which fixes opbeat/opbeat-node#179), but we unfortunately hadn't thought of this use-case back then.

Copy link
Contributor

@Qard Qard left a comment

Choose a reason for hiding this comment

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

Another test for the mimic-response patches would be to verify a transformed stream is transformed as expected. A simple upper-casing transform would work.

What you have here seems sufficient though, if you want to merge and release right away and expand tests in another PR?

// Start simple server that performs got-request on every request
var server = http.createServer(function (req, res) {
got(url).then(function (response) {
t.equal(response.body[0], '\'', 'body should be uncompressed')
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd verify more than just the first character.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If it's gzipped, it will always start with a few magic gzip bytes, so it can never be '. But I could just read the entire file into memory up top and do a complete comparison - that way it's good for sure 😃

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm actually the downside of that is that the test output will display the binary gzipped data if the test fails when mimic-response isn't correctly patched. And that might mess with your shell. The first magic byte in the gzip header is \x1f and will always be displayed as such, so there's no danger of messing up the terminal in that case.

Here's how it looks if I display it all:

image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Continuing the conversation with my self: Actually the dangerous bytes are probably all escaped - just as \x1f, so I guess my concern is invalid 😅

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, ended up testing just the first line and added a test for the total length as well. That should hopefully cover everything 😃

@watson watson merged commit c92ad06 into elastic:master Jul 4, 2018
@watson watson deleted the mimic-response branch July 4, 2018 07:55
trentm added a commit that referenced this pull request Sep 14, 2021
The mechanism to see if an EventEmitter has already been bound
to a run context (as required by the mimic-response instrumentation;
and *only* it) has changed.

This also removes the shimmer.isWrapped() export that was added
in #429 just for this case and is still the only usage of it.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants