Skip to content
New issue

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

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

Already on GitHub? # to your account

DNS resolutions overflow UV pool #47

Closed
mast opened this issue Mar 5, 2018 · 7 comments · Fixed by #69
Closed

DNS resolutions overflow UV pool #47

mast opened this issue Mar 5, 2018 · 7 comments · Fixed by #69
Labels
enhancement 🎁 Rewarded on Issuehunt This issue has been rewarded on Issuehunt help wanted

Comments

@mast
Copy link

mast commented Mar 5, 2018

Issuehunt badges

I found situation when using is-online gives negative consequences on my system.

Preconditions

  • your network is offline
  • you use is-online periodically by timer

What will happen

is-online uses domain name for network checking, it means DNS resolution will be done first. Since network is actually offline, DNS resolution is becoming very long operation, which will block thread of UV pool. Since we run it on regular basis (in my case I ran it every 5 seconds), your UV pool overflows after some time. By default UV pool size is 4.
After UV pool is overflown, all UV operations will be queued (like fs.* functions), which may cause significant delays of their execution. In my case after 20 minutes of operation, queue increased to 50-70 operations, which gave me almost never ending new operations (like fs.readFile or any other).

I'd suggest avoid DNS resolution here (in my project I implemented 8.8.8.8:53 connection instead) or cancel DNS resolution requests when timeout happens.

Information about UV pool can be found here
https://www.future-processing.pl/blog/on-problems-with-threads-in-node-js/


IssueHunt Summary

lukks lukks has been rewarded.

Backers (Total: $30.00)

Submitted pull Requests


Tips


IssueHunt has been backed by the following sponsors. Become a sponsor

@silverwind
Copy link
Collaborator

silverwind commented Mar 5, 2018

This should be solveable without dropping DNS. We need to

  1. Make sure timeout is passed down to is-online (It currently isn't).
  2. Make sure the dgram socket created in is-online is destroyed or recycled on timeout.
  3. Advice users to set timeout lower than the frequency on which they call the function.

@sindresorhus
Copy link
Owner

This sounds like a Node.js bug to me. It should not allow DNS to block the whole pool.

@silverwind
Copy link
Collaborator

public-ip uses dns-socket which is a pure JS DNS implementation so should not be affected by thread pool issues like core dns is.

@sindresorhus
Copy link
Owner

Advice users to set timeout lower than the frequency on which they call the function.

Why?

@issuehunt-oss issuehunt-oss bot added the 💵 Funded on Issuehunt This issue has been funded on Issuehunt label Jun 11, 2019
@IssueHuntBot
Copy link

@issuehunt has funded $30.00 to this issue.


@LuKks
Copy link

LuKks commented Mar 23, 2020

1st process/console: original is-online + original public-ip@v3.0.0
2nd process/console: original is-online + original public-ip@v4.0.0
3rd process/console: modified is-online + modified public-ip@v4.0.0

About the 1st: We can see that the ram usage is increasing and decreasing very quickly which can be unstable due gc. By the way, if you do one simple call to check if it's online then the node process keep running +30s, so imagine that small leak repeated hundreds of time, that's why the ram is "unstable". This is running the current version is-online@8.2.1 without modifications.

About the 2nd: is consuming a lot of resources due a leak in the redundancy (which was added in v4) and also due a problem with dns-socket, we can see the bad performance checking the unstable/laggy output in the 2nd console (also very high cpu/mem), it looks like the garbage collector is really having a bad time there. Remember, this is just the original is-online with public-ip upgraded to v4.

About the 3rd: It's the more stable version that I could achieve. The "leak" related to redundancy is when you try to cancel the query and you get back the result cancelled but in background it continues doing every query with 5s of timeout each. Another problem is the default maxQueries (10000) of dns-socket which is overkill because we just do one query at the same time, also there is a bug with the retries (when you set to 0 then it actually sets to the default 5). The cpu and ram is very stable and it's even lower.

Peek 2020-03-22 23-29

These tests were made without network connection as the original issue.
Just in case, my cpu is very low-end so don't worry too much about the 40% of cpu usage, it actually performs very stable.
Every process were running with:

const isOnline = require('is-online');

let count = 0;

for (let i = 0; i < 1; i++) {
  setInterval(async () => {
    let time = Date.now();
    count++;
    let result = await isOnline({ timeout: 5000 });
    count--;
    time = Date.now() - time;

    console.log('result', result, 'count', count, 'time', time);
  }, 50);
}

@issuehunt-oss
Copy link

issuehunt-oss bot commented Apr 4, 2020

@sindresorhus has rewarded $27.00 to @lukks. See it on IssueHunt

  • 💰 Total deposit: $30.00
  • 🎉 Repository reward(0%): $0.00
  • 🔧 Service fee(10%): $3.00

@issuehunt-oss issuehunt-oss bot added 🎁 Rewarded on Issuehunt This issue has been rewarded on Issuehunt and removed 💵 Funded on Issuehunt This issue has been funded on Issuehunt labels Apr 4, 2020
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
enhancement 🎁 Rewarded on Issuehunt This issue has been rewarded on Issuehunt help wanted
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants