assaf / node-replay

When API testing slows you down: record and replay HTTP responses like a boss
http://documentup.com/assaf/node-replay
MIT License
522 stars 107 forks source link

Replay does not return any data if stream is not immediately read #139

Open nicolasazrak opened 6 years ago

nicolasazrak commented 6 years ago

I was trying to use https://github.com/sindresorhus/got with replay without success. When using replay the responses bodies are always empty. After reading the code of both libraries I discovered that replay returns a response and sets its stream in flowing state (https://nodejs.org/api/stream.html#stream_two_modes). The problem with that mode is that if the content is not immediately read, it will be lost. Libraries using http module expect the stream not to be flowing. This example reproduces the issue:

const replay = require('replay');
const http = require('http');

const req = http.request('http://dev.flowics.com:3000/', res => {
  setImmediate(() => {
    res.setEncoding('utf8');
    res.on('data', chunk => {
      console.log(`${chunk}`);
    });
    res.on('end', () => {
      console.log('No more data in response.');
    });
  });
});

req.end();

Without the setImmediate it works as expected. But it's different to how the http module works. Just removing this line solves the issue.

jeromegn commented 6 years ago

I've also encountered this issue just now. I'm keeping the http.IncomingMessage for a little while and only reading from it later (only if necessary). Without node-replay this is fine, but when using it the body is empty.

I'm wondering if changing it to not resume would break current usage or not. The current test suite fails on my machine. A few more tests fail when removing that response.resume() line, so I assume there would be breakage.

zebulonj commented 5 years ago

This is biting me too. Thanks for tracking down the cause @nicolasazrak.

zebulonj commented 5 years ago

@assaf Is there a reason that Replay is defaulting returned streams to flowing mode, when the default for Node is paused mode? I recognize that it would be a breaking change, but... it seems reasonable that when patching the http library, it's defaults would be preserved.

If you're not inclined to change that default behavior in Replay, would you be open to a pull-request that adds an option to disable the automatic flow of response data? I'd propose defaulting to automatic resume on capture (so it's not a breaking change), but allow users to restore the http default using an interface something like Replay.autoResume = false; (I propose setting that on the Replay object because that's how other options are currently set, i.e., Replay.mode.