Fix the problem reported in #102

[benzino77]

Proper error handling in scanStream method.

[benzino77]

hmmm... that is strange. On my side all tests are passing without errors...

[kylefarris]

Sometimes I have to run the CI tests a few times to get them to pass. Just something weird with the environment it runs in, I think.

[benzino77]

There is a problem with node version, I think. On my side (on my workstation) tests run on node v17 sometimes failing:

Now using node v17.9.1 (npm v8.11.0)
❯ npm run test

> clamscan@2.1.2 test
> make test

npm ERR! code ERESOLVE
npm ERR! ERESOLVE could not resolve
npm ERR!
npm ERR! While resolving: eslint-config-airbnb@15.1.0
npm ERR! Found: eslint@7.25.0
npm ERR! node_modules/eslint
npm ERR!   dev eslint@"^7.25.0" from the root project
npm ERR!   peer eslint@">= 4.12.1" from babel-eslint@10.1.0
npm ERR!   node_modules/babel-eslint
npm ERR!     dev babel-eslint@"^10.1.0" from the root project
npm ERR!   5 more (eslint-config-prettier, eslint-plugin-chai-friendly, ...)
npm ERR!
npm ERR! Could not resolve dependency:
npm ERR! peer eslint@"^3.19.0 || ^4.3.0" from eslint-config-airbnb@15.1.0
npm ERR! node_modules/eslint-config-airbnb
npm ERR!   dev eslint-config-airbnb@"^15.0.1" from the root project
npm ERR!
npm ERR! Conflicting peer dependency: eslint@4.19.1
npm ERR! node_modules/eslint
npm ERR!   peer eslint@"^3.19.0 || ^4.3.0" from eslint-config-airbnb@15.1.0
npm ERR!   node_modules/eslint-config-airbnb
npm ERR!     dev eslint-config-airbnb@"^15.0.1" from the root project
npm ERR!
npm ERR! Fix the upstream dependency conflict, or retry
npm ERR! this command with --force, or --legacy-peer-deps
npm ERR! to accept an incorrect (and potentially broken) dependency resolution.
npm ERR!
npm ERR! See /home/benzino/.npm/eresolve-report.txt for a full report.

npm ERR! A complete log of this run can be found in:
npm ERR!     /home/benzino/.npm/_logs/2022-08-11T08_57_29_795Z-debug-0.log
make: *** [Makefile:5: all] Error 1

on all other node versions (v12, v13, v14, v15, v16) I'm not observing such behavior.

[benzino77]

Kyle are you want me to do something more with this?

[carboneater]

@benzino77 the error you pasted is related to npm 8, where it now enforces peer dependencies strictly

[carboneater]

@benzino77 the error you pasted is related to npm 8, where it now enforces peer dependencies strictly

Good news is I gnawed at this problem over the last few days. It's solved in #106

[benzino77]

@benzino77 the error you pasted is related to npm 8, where it now enforces peer dependencies strictly

Good news is I gnawed at this problem over the last few days. It's solved in #106

That is great! So if your PR will be merged then there is a chance that this one will not fail on tests.

[benzino77]

@kylefarris since #106 was merged could you please rerun tests for this PR? I have no idea how should I do it on my own....

[origooo]

Not sure what the status of this PR is, but it seems to fix the issues with error handling for me. Without this fix, sometimes the EPIPE error is not catchable which breaks my api endpoints somewhat. With the fix it is indeed properly caught.

Anything I can do to help?

[kylefarris]

Hey guys, I'll take a look at this today and run the tests and get it merged in if all is well. Sorry for the delay! Been really busy with work :)

[origooo]

Some more information. I've added a test case which passes a too large stream through scanStream (which was my initial problem). The fix suggested in this PR helps in catching the two types of errors which can occur from that. The errors are either EPIPE or NodeClamError "INSTREAM size limit exceeded".

What I don't understand is that the type of error I end up catching is inconsistent. This is okay since I can look for both types of errors. But do we know where the inconsistency comes from? It would be sweet if we could look for e.g. a "SIZE_LIMIT_EXCEEDED" or so but that would not be possible without fixing the inconsistency. I have seen that you've mentioned in other issue threads that we can look for NodeClamError + data.error.includes("INSTREAM size limit exceeded"). That might be better instead of adding specific error handling for all potential ClamAV errors.

EDIT
I guess the answer to the inconsistency is found in the discussion leading up to this PR.

So to summarise/answer my comment above, we cannot have a "SIZE_LIMIT_EXCEEDED" on streams due to the answer in the link. This PR does, however, fix the error handling of too large streams.

[kylefarris]

Merged! Thanks everyone! I'm going to get dependencies updated and a new version published shortly.