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

Only one HostsResolver per hosts file #28

Merged
merged 5 commits into from
May 7, 2020
Merged

Conversation

szmarczak
Copy link
Owner

Fixes #25

@szmarczak
Copy link
Owner Author

@dylang What do you think about this?

@dylang
Copy link

dylang commented May 6, 2020

Thanks for your quick attention this. This does not solve the memory issue, at least not in Jest. I'll try to dig deeper later, I can't right now.

@dylang
Copy link

dylang commented May 6, 2020

I think it doesn't solve the problem in this part of the code:

this._promise = (async () => {
			if (typeof this._hostsPath !== 'string') {
				return;
			}

			await this._update();

			if (this._error) {
				return;
			}

			watchFile(this._hostsPath, {
				persistent: false
			}, (current, previous) => {
				if (current.mtime > previous.mtime) {
					this._update();
				}
			}).once('error', error => {
				this._error = error;
				this._hosts = {};
			});

			this._promise = null;
		})();

Can watchFile be opt-in instead of opt-out?

What stops watchFile?

@szmarczak
Copy link
Owner Author

What stops watchFile?

Simply pass dnsCache: new CacheableLookup({customHostsPath: false}) or dnsCache: false.

@szmarczak
Copy link
Owner Author

Can watchFile be opt-in instead of opt-out?

Yep, will do this tomorrow.

@dylang
Copy link

dylang commented May 6, 2020

Simply pass dnsCache: new CacheableLookup({customHostsPath: false}) or dnsCache: false.

That prevents it, but once watchFile starts watching, what will stop it from watching, other than killing the process?

Also, unrelated, using fs.watch() is more efficient than fs.watchFile()
https://nodejs.org/docs/latest/api/fs.html#fs_fs_watch_filename_options_listener

@szmarczak
Copy link
Owner Author

what will stop it from watching, other than killing the process?

Unfortunately there are no destructors in JS.
That's why I implemented in this PR some sort of singletons.

Also, unrelated, using fs.watch() is more efficient than fs.watchFile()

Thanks, didn't know that! But this doesn't fix the memory leak either, does it?

@szmarczak szmarczak merged commit 93a2025 into master May 7, 2020
@szmarczak szmarczak deleted the one-hosts-resolver branch May 7, 2020 09:02
@dylang
Copy link

dylang commented May 7, 2020

Unfortunately there are no destructors in JS.

Correct. This is why it might be better to export a method to start and stop the watcher.

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

Successfully merging this pull request may close these issues.

Memory leak each time cachableLookup is imported and constructed.
2 participants