Comments (38)
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:
destroy()
is called which setsdestroyed=true
callback(err)
is called, which would cause the stream to invokedestroy(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.
Related Issues (20)
- Input namespace parsing HOT 2
- Stack overflow on multipart upload with domains HOT 24
- fs rename ENOENT HOT 6
- Error: ENOENT, open '......' when using uploadDir HOT 2
- can you support koa? HOT 1
- AWS upload. HOT 1
- Question - How do I delete the (temp) files on the server? HOT 2
- update multiparty to 4.1.3 HOT 3
- "High severity" security vulnerability affecting latest release (v2.0.0) HOT 1
- stream ended unexpectedly HOT 2
- Error: stream ended unexpectedly HOT 13
- About article last word HOT 1
- upload file save local HOT 3
- File upload from android device to node server HOT 4
- Uploading a file from Postman/Futter HOT 2
- Unlink file when request timed out HOT 2
- I have the same issue if I don't send any body from post man to any service. What is the wrong ?? and how to fix it ?? HOT 2
- TypeError: os.tmpDir is not a function HOT 2
- req.files is an empty object HOT 10
Recommend Projects
-
React
A declarative, efficient, and flexible JavaScript library for building user interfaces.
-
Vue.js
🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
-
Typescript
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
-
TensorFlow
An Open Source Machine Learning Framework for Everyone
-
Django
The Web framework for perfectionists with deadlines.
-
Laravel
A PHP framework for web artisans
-
D3
Bring data to life with SVG, Canvas and HTML. 📊📈🎉
-
Recommend Topics
-
javascript
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
-
web
Some thing interesting about web. New door for the world.
-
server
A server is a program made to process requests and deliver data to clients.
-
Machine learning
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
-
Visualization
Some thing interesting about visualization, use data art
-
Game
Some thing interesting about game, make everyone happy.
Recommend Org
-
Facebook
We are working to build community through open source technology. NB: members must have two-factor auth.
-
Microsoft
Open source projects and samples from Microsoft.
-
Google
Google ❤️ Open Source for everyone.
-
Alibaba
Alibaba Open Source for everyone
-
D3
Data-Driven Documents codes.
-
Tencent
China tencent open source team.
from connect-multiparty.