m4nuC / async-busboy

Promise based multipart form parser for KoaJS
MIT License
167 stars 58 forks source link

node 14 #42

Closed FredericHeem closed 3 years ago

FredericHeem commented 4 years ago

It seems that node 14 has some issue with this lib, the following code hangs:

const { files, fields } = await asyncBusboy(context.req, config);

node 13 is fine. Thanks.

alqu commented 4 years ago

Same here. Any solution in sight?

amit777 commented 3 years ago

Also noticed upload issue on node14. Works on node 12. Using v1.0.1

amit777 commented 3 years ago

I've been debugging this. While I thought the code would always hang, I did actally see it succesfully work twice while I was putting console logs in.. I thought maybe the console.log was somehow causing it to start working, but it doesn't seem to be the case.. so it seems like it will fail most of the time but may occasionally work.

amit777 commented 3 years ago

ok.. I think i've found the culprit. busboy's "close" event seems to be firing before "finished". When this happens, the finished event gets unsubscribed to, and the Promise is not resolved. I'm not sure if some assumption changed in Node14 streams about the order of events.

busboy.on('close', cleanup);

amit777 commented 3 years ago

Hi @FredericHeem and @alqu would you mind testing my fork with the fixes here: https://github.com/amit777/async-busboy

It seems to work for me.

imox2 commented 3 years ago

@amit777 yep, thats the culprit. I too found that

Screenshot 2020-09-18 at 2 17 29 AM

Also, don't you think busboy.on('close', cleanup) is required for some other case. So maybe removing that will solve this case but break anything else?

Also, I was more interested in finding why is close called early. Maybe something changed in node 14

amit777 commented 3 years ago

in my pull request I did remove the .on('close') since it only calls cleanup(). Cleanup looks like it would get call anyway later by another event, so I think it may not be impactful, but I'm not sure.

Out of curious, what tool did you use that generated that output from your screenshot?

ogarich89 commented 3 years ago

Same problem. Had to use @koa/multer =(

alqu commented 3 years ago

Hi @FredericHeem and @alqu would you mind testing my fork with the fixes here: https://github.com/amit777/async-busboy

It seems to work for me.

I tested it locally and on a production server - it works fine, no problems so far.

alqu commented 3 years ago

Thanks for merging @amit777. Please update the npm package :-)

FredericHeem commented 3 years ago

@amit777 thanks a lot, confirmation from my side your fork is working fine. Waiting for the package to be published.

lehmamic commented 3 years ago

have the same problem, waiting as well...

fjeddy commented 3 years ago

Guess we can consider this repo dead.

amit777 commented 3 years ago

@m4nuC would it be possible to publish the updated package on npm? It looks like a lot of testers confirmed it works.

m4nuC commented 3 years ago

@amit777 on it. Sorry for the delay

m4nuC commented 3 years ago

1.1.0 published to NPM. Apologies to everyone for the delay

Sceat commented 3 years ago

still hang on node 16

Uzlopak commented 2 years ago

Do you set autoDestroy to false?