Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: handle ping timeout error after client created #136

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jaimepk27
Copy link

Fix for timeout error on ping request using remote server.

Explanation

The timeout error is thrown after the client is created and the promise is neither resolved nor rejected.

To fix this, the ping method should handle the timeout error after the client is created.

Test scenario

First, run a clamav server.

services:
  clamav:
    image: tiredofit/clamav
    ports:
      - 3310:3310
    volumes:
      - clamav_data:/data
      - clamav_logs:/logs

volumes:
  clamav_data:
  clamav_logs:

Then, block traffic for clamav server.

sudo iptables -D INPUT -p tcp --dport 3310 -j DROP
sudo iptables -D OUTPUT -p tcp --dport 3310 -j DROP

And finally, try to connect to clamav server.

try {
    const client = await new NodeClam().init({
            clamdscan: {
                host: 'localhost',
                port: 3310,
                timeout: 2_000,
            },
            preference: 'clamdscan',
        });
} catch(err) {
    console.error('Failed to create ClamAV client', err);
}

@jaimepk27
Copy link
Author

A suggestion to improve ping method. It would be possible to add a config property pingTimeout to use it in ping requests. This way, there will be two timeouts for scan and ping requests.

@kylefarris
Copy link
Owner

Thank you for your contribution!

We need to figure out a way to make a working test as part of our test suite before this can be merged in. Give it a go and see if you can get a test written for this scenario.

Also, documentation would need to be updated if you want to add a pingTimeout config property. How would the pingTimeout configuration be implemented?

@kylefarris kylefarris self-assigned this Oct 24, 2024
@kylefarris kylefarris added the bug label Oct 24, 2024
@jaimepk27
Copy link
Author

Thank you for your contribution!

We need to figure out a way to make a working test as part of our test suite before this can be merged in. Give it a go and see if you can get a test written for this scenario.

Of course, I will try to add a test for timeout errors.

Also, documentation would need to be updated if you want to add a pingTimeout config property. How would the pingTimeout configuration be implemented?

Ok, I will update the docs as well. I was thinking about adding pingTimeout to the init options and then use it as param in _initSocket.

@kylefarris
Copy link
Owner

That all sounds great! Looking forward to seeing the feature added!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants