Skip to content

Commit 78515b9

Browse files
fengmk2fengmk2
authored
fix: check writeStream destroyed before send request (#460)
Only check on Node.js < 18 closes #459
1 parent 0d9f662 commit 78515b9

6 files changed

Lines changed: 51 additions & 32 deletions

File tree

‎examples/timing.cjs‎

Lines changed: 7 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -3,21 +3,24 @@ const urllib = require('..');
33
const url = process.argv[2] || 'https://cnodejs.org';
44
console.log('timing: %s', url);
55

6-
const count = 10000;
6+
const count = 5;
77

88
async function request(index) {
99
if (index === count) {
1010
return;
1111
}
12-
const res = await urllib.request(url, {
12+
const res = await urllib.request(url + '?index=' + index, {
1313
// data: { wd: 'nodejs' },
14+
dataType: 'json',
1415
});
1516
console.log('---------------------------');
1617
console.log('No#%d: %s, content size: %d, requestUrls: %o, socket: %o, rt: %o',
1718
index, res.statusCode, res.data.length, res.res.requestUrls, res.res.socket, res.res.rt);
18-
console.log(res.res.timing, res.headers);
19+
console.log(res.res.timing);
20+
// console.log(res.res.timing, res.headers);
21+
// console.log(res.data);
1922
index++;
2023
setImmediate(request.bind(null, index));
2124
}
2225

23-
request(1);
26+
request(0);

‎src/HttpAgent.ts‎

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -37,8 +37,11 @@ export class HttpAgent extends Agent {
3737
/* eslint node/prefer-promises/dns: off*/
3838
const _lookup = options.lookup ?? dns.lookup;
3939
const lookup: LookupFunction = (hostname, dnsOptions, callback) => {
40-
_lookup(hostname, dnsOptions, (err, address, family) => {
41-
if (err) return callback(err, address, family);
40+
_lookup(hostname, dnsOptions, (err, ...args: any[]) => {
41+
// address will be array on Node.js >= 20
42+
const address = args[0];
43+
const family = args[1];
44+
if (err) return (callback as any)(err, address, family);
4245
if (options.checkAddress) {
4346
// dnsOptions.all set to default on Node.js >= 20, dns.lookup will return address array object
4447
if (typeof address === 'string') {
@@ -55,7 +58,7 @@ export class HttpAgent extends Agent {
5558
}
5659
}
5760
}
58-
callback(err, address, family);
61+
(callback as any)(err, address, family);
5962
});
6063
};
6164
super({

‎src/HttpClient.ts‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -58,6 +58,8 @@ function noop() {
5858
}
5959

6060
const debug = debuglog('urllib:HttpClient');
61+
// Node.js 14 or 16
62+
const isNode14Or16 = /v1[46]\./.test(process.version);
6163

6264
export type ClientOptions = {
6365
defaultArgs?: RequestOptions;
@@ -543,6 +545,9 @@ export class HttpClient extends EventEmitter {
543545
res = Object.assign(response.body, res);
544546
}
545547
} else if (args.writeStream) {
548+
if (isNode14Or16 && args.writeStream.destroyed) {
549+
throw new Error('writeStream is destroyed');
550+
}
546551
if (args.compressed === true && isCompressedContent) {
547552
const decoder = contentEncoding === 'gzip' ? createGunzip() : createBrotliDecompress();
548553
await pipelinePromise(response.body, decoder, args.writeStream);

‎test/fixtures/foo.txt‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
😄foo😭.txt content

‎test/options.files.test.ts‎

Lines changed: 13 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -75,15 +75,17 @@ describe('options.files.test.ts', () => {
7575

7676
it('should upload multi files: Record<field, string> success with default POST method', async () => {
7777
const file = path.join(__dirname, 'cjs', 'index.js');
78-
const txt = path.join(__dirname, 'fixtures', '😄foo😭.txt');
78+
// const txt = path.join(__dirname, 'fixtures', '😄foo😭.txt');
79+
const txt = path.join(__dirname, 'fixtures', 'foo.txt');
7980
const json = path.join(__dirname, '..', 'package.json');
8081
const stat = await fs.stat(file);
8182
const jsonStat = await fs.stat(json);
8283
const txtStat = await fs.stat(txt);
8384
const response = await urllib.request(`${_url}multipart`, {
8485
files: {
8586
file,
86-
'😄foo😭.js': txt,
87+
// '😄foo😭.js': txt,
88+
'foo.js': txt,
8789
json,
8890
},
8991
dataType: 'json',
@@ -92,9 +94,12 @@ describe('options.files.test.ts', () => {
9294
// console.log(response.data);
9395
assert.equal(response.data.method, 'POST');
9496
assert.match(response.data.headers['content-type'], /^multipart\/form-data;/);
95-
assert.equal(response.data.files['ð\x9F\x98\x84fooð\x9F\x98­.js'].filename, 'ð\x9F\x98\x84fooð\x9F\x98­.txt');
96-
assert.equal(response.data.files['ð\x9F\x98\x84fooð\x9F\x98­.js'].mimeType, 'text/plain');
97-
assert.equal(response.data.files['ð\x9F\x98\x84fooð\x9F\x98­.js'].size, txtStat.size);
97+
// assert.equal(response.data.files['ð\x9F\x98\x84fooð\x9F\x98­.js'].filename, 'ð\x9F\x98\x84fooð\x9F\x98­.txt');
98+
// assert.equal(response.data.files['ð\x9F\x98\x84fooð\x9F\x98­.js'].mimeType, 'text/plain');
99+
// assert.equal(response.data.files['ð\x9F\x98\x84fooð\x9F\x98­.js'].size, txtStat.size);
100+
assert.equal(response.data.files['foo.js'].filename, 'foo.txt');
101+
assert.equal(response.data.files['foo.js'].mimeType, 'text/plain');
102+
assert.equal(response.data.files['foo.js'].size, txtStat.size);
98103
assert.equal(response.data.files.file.filename, 'index.js');
99104
assert.equal(response.data.files.file.mimeType, 'application/javascript');
100105
assert.equal(response.data.files.file.size, stat.size);
@@ -209,7 +214,9 @@ describe('options.files.test.ts', () => {
209214
it('should upload a file with args.data success', async () => {
210215
const stat = await fs.stat(__filename);
211216
const largeFormValue = await fs.readFile(__filename, 'utf-8');
212-
const txt = path.join(__dirname, 'fixtures', '😄foo😭.txt');
217+
// emoji not work on windows node.js >= 20
218+
// const txt = path.join(__dirname, 'fixtures', '😄foo😭.txt');
219+
const txt = path.join(__dirname, 'fixtures', 'foo.txt');
213220
const txtValue = await fs.readFile(txt, 'utf-8');
214221
const response = await urllib.request(`${_url}multipart`, {
215222
method: 'HEAD',

‎test/options.writeStream.test.ts‎

Lines changed: 19 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -100,30 +100,30 @@ describe('options.writeStream.test.ts', () => {
100100
});
101101

102102
// writeStream only work with error handle on Node.js >= 18
103-
if (!process.version.startsWith('v14.') && !process.version.startsWith('v16.')) {
104-
it('should throw request error when writeStream error', async () => {
105-
const tmpfile = join(__dirname, 'not-exists-dir', 'foo.txt');
106-
const writeStream = createWriteStream(tmpfile);
107-
let writeStreamError = false;
108-
writeStream.on('error', () => {
109-
writeStreamError = true;
103+
// bugfix: https://github.com/node-modules/urllib/issues/459
104+
it('should throw request error when writeStream error', async () => {
105+
const tmpfile = join(__dirname, 'not-exists-dir', 'foo.txt');
106+
const writeStream = createWriteStream(tmpfile);
107+
let writeStreamError = false;
108+
writeStream.on('error', () => {
109+
writeStreamError = true;
110+
});
111+
await assert.rejects(async () => {
112+
await urllib.request(_url, {
113+
writeStream,
110114
});
111-
await assert.rejects(async () => {
112-
await urllib.request(_url, {
113-
writeStream,
114-
});
115-
}, (err: any) => {
116-
// console.error(err);
115+
}, (err: any) => {
116+
// only Node.js >= 18 has stream.emitError
117+
if (err.message !== 'writeStream is destroyed') {
117118
assert.equal(err.name, 'Error');
118119
assert.equal(err.code, 'ENOENT');
119120
assert.match(err.message, /no such file or directory/);
120-
assert.equal(writeStream.destroyed, true);
121-
return true;
122-
});
123-
assert.equal(writeStream.destroyed, true);
124-
assert.equal(writeStreamError, true);
121+
}
122+
return true;
125123
});
126-
}
124+
assert.equal(writeStream.destroyed, true);
125+
assert.equal(writeStreamError, true);
126+
});
127127

128128
it('should end writeStream when server error', async () => {
129129
const writeStream = createWriteStream(tmpfile);

0 commit comments

Comments
 (0)