req.files empty on nodejs 14 about connect-multiparty HOT 38 CLOSED

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.

from connect-multiparty.

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

from connect-multiparty.

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.

from connect-multiparty.

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.

from connect-multiparty.

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

from connect-multiparty.

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

from connect-multiparty.

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.

from connect-multiparty.

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

from connect-multiparty.

@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.

from connect-multiparty.

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.

from connect-multiparty.

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

from connect-multiparty.

@dougwilson thanks 👍

from connect-multiparty.

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?

from connect-multiparty.

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

from connect-multiparty.

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).

from connect-multiparty.

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.

from connect-multiparty.

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?

from connect-multiparty.

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.

from connect-multiparty.

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?

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

If autoDestroy: true then the callback will use destroy(err) to error the stream, which won't emit an error with the overriden destroy impl.

from connect-multiparty.

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.

from connect-multiparty.

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.

from connect-multiparty.

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?

from connect-multiparty.

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.

from connect-multiparty.

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

from connect-multiparty.

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.

from connect-multiparty.

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.

from connect-multiparty.

Related nodejs/node@311e12b

from connect-multiparty.

the http request stream is suddenly emitting close event

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

from connect-multiparty.

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).

from connect-multiparty.

@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.

from connect-multiparty.

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+.

from connect-multiparty.

+1 for some updates

from connect-multiparty.

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

from connect-multiparty.

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

from connect-multiparty.

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

from connect-multiparty.

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

from connect-multiparty.

Fixed in pillarjs/multiparty#226

from connect-multiparty.

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

from connect-multiparty.