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: only emit 'end' after unzip is done writing #1766

Merged
merged 1 commit into from
Aug 15, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
6 changes: 3 additions & 3 deletions src/node/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -463,13 +463,13 @@ Request.prototype._pipeContinue = function (stream, options) {
stream.emit('error', error);
});
res.pipe(unzipObject).pipe(stream, options);
// don't emit 'end' until unzipObject has completed writing all its data.
unzipObject.once('end', () => this.emit('end'));
} else {
res.pipe(stream, options);
res.once('end', () => this.emit('end'));
}

res.once('end', () => {
this.emit('end');
});
});
return stream;
};
Expand Down
55 changes: 55 additions & 0 deletions test/node/pipe.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@ const app = express();
const fs = require('fs');
const bodyParser = require('body-parser');
let http = require('http');
const zlib = require('zlib');
const { pipeline } = require('stream');

if (process.env.HTTP2_TEST) {
http = require('http2');
Expand All @@ -17,6 +19,13 @@ app.get('/', (request_, res) => {
fs.createReadStream('test/node/fixtures/user.json').pipe(res);
});

app.get('/gzip', (request_, res) => {
res.writeHead(200, {
'Content-Encoding': 'gzip'
});
fs.createReadStream('test/node/fixtures/user.json').pipe(new zlib.createGzip()).pipe(res);
});

app.get('/redirect', (request_, res) => {
res.set('Location', '/').sendStatus(302);
});
Expand Down Expand Up @@ -102,6 +111,52 @@ describe('request pipe', () => {
request_.pipe(stream);
});

it('should act as a readable stream with unzip', (done) => {
const stream = fs.createWriteStream(destinationPath);

let responseCalled = false;
const request_ = request.get(base + '/gzip');
request_.type('json');

request_.on('response', (res) => {
res.status.should.eql(200);
responseCalled = true;
});
stream.on('finish', () => {
JSON.parse(fs.readFileSync(destinationPath)).should.eql({
name: 'tobi'
});
responseCalled.should.be.true();
done();
});
request_.pipe(stream);
});

it('should act as a readable stream with unzip and node.stream.pipeline', (done) => {
const stream = fs.createWriteStream(destinationPath);

let responseCalled = false;
const request_ = request.get(base + '/gzip');
request_.type('json');

request_.on('response', (res) => {
res.status.should.eql(200);
responseCalled = true;
});
// pipeline automatically ends streams by default. Since unzipping introduces a transform stream that is
// not monitored by pipeline, we need to make sure request_ does not emit 'end' until the unzip step
// has finished writing data. Otherwise, we'll either end up with truncated data or a 'write after end' error.
pipeline(request_, stream, function (err) {
(!!err).should.be.false();
responseCalled.should.be.true();

JSON.parse(fs.readFileSync(destinationPath)).should.eql({
name: 'tobi'
});
done();
});
});

it('should follow redirects', (done) => {
const stream = fs.createWriteStream(destinationPath);

Expand Down