Regression in Formidable v3, which can crash a server

[laurent22]

Using Formidable 3.5.1

Since we upgraded from v2 to Formidable v3, our server is crashing every few days, always with these errors:

server_1  | 2023-10-29 10:08:51: [error] App: 174.194.132.166 FormidableError: Could not parse form: Request aborted
server_1  |     at IncomingMessage.<anonymous> (/home/joplin/packages/server/node_modules/formidable/dist/index.cjs:1223:21)
server_1  |     at IncomingMessage.emit (node:events:517:28)
server_1  |     at IncomingMessage._destroy (node:_http_incoming:224:10)
server_1  |     at _destroy (node:internal/streams/destroy:109:10)
server_1  |     at IncomingMessage.destroy (node:internal/streams/destroy:71:5)
server_1  |     at abortIncoming (node:_http_server:766:9)
server_1  |     at socketOnClose (node:_http_server:760:3)
server_1  |     at Socket.emit (node:events:529:35)
server_1  |     at TCP.<anonymous> (node:net:350:12) {
server_1  |   code: 1002,
server_1  |   httpCode: 500
server_1  | }

Followed by this one:

server_1  | Error: Cannot call write after a stream was destroyed
server_1  |     at new NodeError (node:internal/errors:405:5)
server_1  |     at node:internal/fs/streams:406:23
server_1  |     at FSReqCallback.wrapper [as oncomplete] (node:fs:829:5)
ubuntu_server_1 exited with code 1

And the server crashes.

The code to parse the form content is relatively straightforward:

https://github.com/laurent22/joplin/blob/49c1c9aa652aff50560ab5f46c9bcdfccbac6409/packages/server/src/utils/requestUtils.ts#L73

As I understand the error happens in an internal stream handler, which means it's impossible to catch and ends up crashing the server. I have searched about this error message and I see that it was addressed in v1.x, but it definitely seems to be back. We didn't have this crash when using Formidable v1 or v2, but a few days after we upgraded to v3 it started happening.

Any idea if something can be done about it?

[laurent22]

By the way it's normal that the tests for this package don't even pass? It looks like there's not one good commit in the git history.

[laurent22]

Well I'm going to assume this project is no longer maintained - releases with regressions that crash servers, no support whatsoever, all test units fail but nobody cares. I'm going to revert back to v2 and will eventually migrate to a more sane library.

[tommyhtran]

I couldn't reproduce the issue on my end, it'd be very helpful if you can give something that can reliably reproduce it. I can abort my request and get the first error, but I haven't seen the second error.

Based off a quick Google search, it seems that other people have gotten the second error with other packages when they have some sort of filesystem issue, usually a lack of disk space. Since you said your server crashes every few days, this might be something to look into. Though, it wouldn't make sense that it only affects v3.

I'm not very familiar with the codebase, but I did notice a discrepancy in the write() method of the VolatileFile and PersistentFile classes. The PersistentFile class doesn't check if the stream was destroyed, unlike the VolatileFile class. I assume checking if the stream is closed is probably good enough?

formidable/src/VolatileFile.js

Line 57 in 1699ec6

if (this._writeStream.closed || this._writeStream.destroyed) {

formidable/src/PersistentFile.js

Line 56 in 1699ec6

if (this._writeStream.closed) {

[tunnckoCore]

@laurent22 sorry for that.

It's true it's not much maintained, and there are reasons, but we are primarily just two. I'm not currently using it but we have a several updates coming.

We are in spikes. The main reason to me is that i'm not in Nodejs much in the past years, and that they merged Busboy into Nodejs 20, so there's mostly no reasons for other external packages. Plus, serverless is everywhere. But completely agree, we'll come back.

By the way it's normal that the tests for this package don't even pass? It looks like there's not one good commit in the git history.

Yeah 😄 that's for quite some time. Something messed when we were adding and removing old "integration" tests from the very first codebase.

[tunnckoCore]

@laurent22

Meanwhile, you can try https://www.npmjs.com/package/formidable-mini - it's bare bone and probably the v4. It's just the parser with a thin wrapper on top,a lot more spec- and Web APIs- compliant.

The exposed API is minimal and also compatible with the current Formidable APIs - async parse and async formData.

[laurent22]

@tunnckoCore and @tommyhtran, many thanks for getting back to me, I definitely understand how tricky it can be to maintain an open source project. For now we reverted to Formidable v2 and didn't experience any crashes anymore so I think we'll keep it safe and stay with this, especially since it already has everything we need.

The mini version seems interesting though - is that something you plan to support or more like a temporary package before v4?

[tunnckoCore]

@laurent22 sure v2 sounds good, at least it's not v1, haha.

Regarding the mini. Since the whole ecosystem is moving towards Web APIs and a lot has changed in the past few years, i think it's good enough. It's literally just the OG parser and a thin layer on top to have at least some compatibility with the current formidable APIs.

Still don't know if it gonna be the v4, but i don't see why not. I plan to move it to the org and start doing (or moving from here) tests on it.