fix: ensure correct streaming when using mimic-response#429
fix: ensure correct streaming when using mimic-response#429watson merged 7 commits intoelastic:masterfrom
Conversation
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
|
Btw, I discovered that this isn't the first time we've had issues with |
Qard
left a comment
There was a problem hiding this comment.
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') |
There was a problem hiding this comment.
I'd verify more than just the first character.
There was a problem hiding this comment.
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 😃
There was a problem hiding this comment.
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:
There was a problem hiding this comment.
Continuing the conversation with my self: Actually the dangerous bytes are probably all escaped - just as \x1f, so I guess my concern is invalid 😅
There was a problem hiding this comment.
Ok, ended up testing just the first line and added a test for the total length as well. That should hopefully cover everything 😃
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.
The
mimic-responsemodule exposes a single function with the following API:If someone is calling
mimicResponsewhere thefromStreamhave had its emitter functions bound usingbindEmitter(fromStream), thetoStreamwill wrongly bind all its event listeners to thefromStream.This becomes especially problematic in case the data is transformed either in the
toStreamor in betweenfromStreamandtoStream, eg:In those cases, the data emitted by
toStreamwill actually be the un-transformed data coming from thefromStream.This commit patches
mimic-responseso that the event emitters oftoStreamruns in the expected context.Fixes #423