req.files empty on nodejs 14

[barisusakli]

I have a feeling this might be a nodejs 14 issue, maybe related to nodejs/node#33050 not sure.

Same code works on node 12.x. On nodejs 14 req.files is just an empty object. Simple app to reproduce below.

const express = require('express')
const path = require('path')
const multi = require('connect-multiparty')
const multiMiddleware = multi();
const app = express()
const port = 3000

app.get('/', (req, res) => res.sendFile(path.resolve(__dirname, './index.html')))

app.post('/upload', multiMiddleware, function (req, res) {
    console.log('got upload', req.files);
    res.send('ok');
});

app.listen(port, () => console.log(`Example app listening at http://localhost:${port}`))

[dougwilson]

Thanks for the report. It does indeed seem to be that issue. And it looks like there is already a PR with a fix on the Node.js repo.

[barisusakli]

14.1.0 got released and above issue was closed but I am still getting the same issue 🤔 https://github.com/nodejs/node/blob/master/doc/changelogs/CHANGELOG_V14.md#14.1.0

[dougwilson]

Thanks. I just downloaded and testing 14.1.0 and the issue is still something wrong in the fd-slicer dependency, which my understanding of #33050 was it was supposed to fix that (nodejs/node#33050 (comment)). I will take a deeper look.

[barisusakli]

@dougwilson thanks 👍

[ronag]

Does this library anywhere along the dependency chain use stream.pipeline or stream.finished? I can't see that it does, in which case the referenced issue(s) should not be related to this?

[ronag]

Could this be related to fd-slicer and the fact that Node v14 set autoDestroy on streams to true by default (was false before)?

[dougwilson]

No, it does not. It seems there are two main issues: (1) the error from writing too much to fd-slicer no longer comes back in Node.js 14+ (https://github.com/andrewrk/node-fd-slicer/blob/master/index.js#L156) which causes this module to stall and (2) the http request stream is suddenly emitting close event, which is closing the form before file writing has completed.

I can fix (2) in multiparty, but struggling on how to fix (1).

[dougwilson]

Could this be related to fd-slicer and the fact that Node v14 set autoDestroy on streams to true by default (was false before)?

Hmm, I just tried setting autoDestroy: false on all the various streams created in multiparty and the error from fd-slicer is still not getting emitted.

[ronag]

No, it does not. It seems there are two main issues: (1) the error from writing too much to fd-slicer no longer comes back in Node.js 14+ (andrewrk/node-fd-slicer:index.js@master#L156 )

Yea, that's a problem. Calling destroy() without error and then invoking the callback is a bug and won't work anymore. That really should be fixed in fd-slicer... somehow 😞

EDIT: I guess we could maybe somehow add some kind of legacy fallback hack in Node but it won't be pretty.

the http request stream is suddenly emitting close event

Is it incorrectly emitting it?

[dougwilson]

Yea, that's a problem. Calling destroy() without error and then invoking the callback is a bug and won't work anymore. That really should be fixed in fd-slicer... somehow 😞

Hmm, is that an error? The .destroy of that module is not from Node.js, and its own destroy does not accept any argument, so not sure how that could be a bug?

Is it incorrectly emitting it?

I'm not sure yet, but that is the primary reason why the files are always empty here.

[ronag]

Hmm, is that an error? The .destroy of that module is not from Node.js, and its own destroy does not accept any argument, so not sure how that could be a bug?

So what happens here is:

1. destroy() is called which sets destroyed=true
2. callback(err) is called, which would cause the stream to invoke destroy(err), but because of 1 becomes a noop.

As far as I can see there is no way around this without modifying fd-slicer.

[dougwilson]

Hm, good point. Are you sure it doesn't emit error if you set autoDestroy: false on fd-slicer?

Yea. I even modified fd-slicer source to force the autoDestroy setting to false, and still no error emitted. I added a trace into the .destroy method of fd-slicer and it is never called with an error argument at any point.

[dougwilson]

which because of 1 becomes a noop.

Where does that happen? I only saw in the Node.js source it looking at the destroyed property inside the writablestate object, not on the writable itself?

[ronag]

https://github.com/nodejs/node/blob/master/lib/_stream_writable.js#L398 calls https://github.com/nodejs/node/blob/master/lib/internal/streams/destroy.js#L135 which has a !destroyed check.

[dougwilson]

Right, but that is on the writablestate object, right? Not the writable object.

[ronag]

Right, but that is on the writablestate object, right? Not the writable object.

Which is a setter that modifies the state:
https://github.com/nodejs/node/blob/master/lib/_stream_writable.js#L708

I think it might be possible to remove this condition in Node which would resolve this issue if autoDestroy: false, but that's not the case unfortunately.

[dougwilson]

Ah. I was also looking at the Node.js docs and it looks like the .destory and .destroyed was added to the writable stream in Node.js 8, so the Node.js's before that would not have been using those method names/properties I guess, which is probably why fd-slicer is using them for it's own state tracking.

[ronag]

Related nodejs/node@311e12b

[ronag]

the http request stream is suddenly emitting close event

I'm still curios about what exactly you determine has changed here in v14.

[dougwilson]

I'm still curios about what exactly you determine has changed here in v14.

Sorry, that is my fault, I misspoke; the stream which is emitted close is the form stream (a writable). Using autoClose false fixes that (as multiparty module opens fds and so the writable shouldn't close until the underlying fds it is writing to is closed).

[dougwilson]

Anyway @barisusakli , it sounds like the breakage here is expected and the path forward will be I likely need to rewrite multiparty to remove the fd-slicer dependency. I will try to get that done this week.

[ronag]

@dougwilson I've made a fork of fd-slicer fixing compability issues in case that's any help for you https://github.com/ronag/node-fd-slicer.

[dougwilson]

Thank you, but it is not of use to this module without support down to Node.js 0.10, as this module and multiparty support those. Ideally I would like to release this fix as a patch version vs a major version update.

[StephenLynx]

Maybe you could drop support to 0.10? I mean, that's really, really, really old and reached EOL for a while now. It's really hard to keep support to things that are too old. Or maybe you could keep MP 4 supporting those and up to 12 and MP 5 supporting node 14+.

[julianlam]

Hi @dougwilson, just a (very) gentle reminder about this issue 😊

[sw360cab]

+1 for some updates

[dougwilson]

Hey @julianlam , @sw360cab , and everyone, sorry for the lack of updates; some life events occurred and I just got back into GitHub / open source things a couple days ago. I pretty much have all the fixes ready for this, and just working to get them published out asap. I am setting myself a timeline for this particular issue to be done by this weekend, if that helps.

[sw360cab]

Hi @dougwilson sorry for hearing a mix of bad and good news. This is a though period.

I just wanted some news to decide whether to go for another path or wait to any fix. A couple week for me is a good timeline.
In my case it is a devel matters since test/prod stages are running code within a container locked into Nodejs v.13.

Thanks for your help.
If I may help, test or review let me know

[StephenLynx]

@sw360cab I know this adds literally nothing to the discussion but I have a morbid curiosity: why are you running node 13 in production?

[sw360cab]

@StephenLynx simply because I have containerized applications which base images are still aligned with Node v13

[shubhamdixit863]

Its a node version issue ,i had node 14 installed and it was not working ,so I switched to node 12 and it worked

[SharathVaddireddy]

Its a node version issue ,i had node 14 installed and it was not working ,so I switched to node 12 and it worked

Yes, it is i have node v14.4.0 which gives an empty object in req.files

[barisusakli]

Fixed in pillarjs/multiparty#226

[dougwilson]

For those following what @barisusakli is referring to, multiparty released 4.2.2 with the fix. This module defines the dependency as ~4.2.1 so it will get picked up automatically. I will release a patch version of this module out soon to follow (by bumping that to ~4.2.2 to force the upgrade) but that's just for me to cut down on incoming issues :) It is out and useable now :O

[sw360cab]

@dougwilson Great news! I have just tested and I can confirm it works. For those already working with the previous version, I suggest to clean your environment by removing multiparty from node-modules directory and the relative multiparty section from package-lock.json. They may be unnecessary steps but they helped me to clean my environment once for all.

[barisusakli]

@dougwilson can you bump the version to force the upgrade?

[webLiang]

multiparty在后续版本已经修复
可以在package.json 添加
"resolutions": {
"connect-multiparty/multiparty": "4.2.3"
},