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

do not periodically re-read /etc/hosts by default #11943

Merged
merged 2 commits into from Dec 23, 2021

Conversation

mostroverkhov
Copy link
Contributor

Motivation:

After #11922, by default io.netty.resolver.DefaultHostsFileEntriesResolver synchronously reads /etc/hosts every 60 seconds, while prior above change It read /etc/hosts once upon instantiation.

Modification:

By default io.netty.resolver.DefaultHostsFileEntriesResolver reads /etc/hosts once upon instantiation. This corresponds to io.netty.hostsFileRefreshInterval = 0

Negative values for io.netty.hostsFileRefreshInterval are forbidden

Result:

By default io.netty.resolver.DefaultHostsFileEntriesResolver reads /etc/hosts once upon instantiation, which corresponds to behavior before #11922

…osts by default

Motivation:

After netty#11922, by default `io.netty.resolver.DefaultHostsFileEntriesResolver` synchronously reads `/etc/hosts` every 60 seconds, while before It read `/etc/hosts` once upon instantiation.

Modification:

By default `io.netty.resolver.DefaultHostsFileEntriesResolver` reads `/etc/hosts` once upon instantiation. This corresponds to io.netty.hostsFileRefreshInterval = 0

Negative values for io.netty.hostsFileRefreshInterval are forbidden

Result:

`io.netty.resolver.DefaultHostsFileEntriesResolver` reads `/etc/hosts` once upon instantiation, which corresponds to default behaviour prior to netty#11922
@mostroverkhov
Copy link
Contributor Author

I am wondering why DefaultHostsFileEntriesResolver was changed In first place - there exists HostsFileEntriesResolver interface, and DnsNameResolverBuilder may be configured externally with any impl: periodic updates, filesystem watcher etc.

@@ -47,7 +47,7 @@

static {
DEFAULT_REFRESH_INTERVAL = SystemPropertyUtil.getLong(
"io.netty.hostsFileRefreshInterval", TimeUnit.SECONDS.toNanos(60));
"io.netty.hostsFileRefreshInterval", /*nanos*/0);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

original pr had io.netty.hostsFileRefreshInterval as nanos - maybe seconds are more appropriate?

@normanmaurer
Copy link
Member

@mostroverkhov what is wrong with re-reading by default ?

@violetagg
Copy link
Member

violetagg commented Dec 22, 2021

@mostroverkhov what is wrong with re-reading by default ?

@normanmaurer This is my perspective: currently with this re-reading the event loop can be blocked. If you are on the server and you want to make a remote call you need always to offload to another thread in order to not block.

It was ok to exclude this one

builder.allowBlockingCallsInside(
"io.netty.resolver.HostsFileEntriesProvider$ParserImpl",
"parse");

because you can take care for the initialisation of io.netty.resolver.DefaultHostsFileEntriesResolver#DefaultHostsFileEntriesResolver(io.netty.resolver.HostsFileEntriesProvider.Parser, long) (i.e. do this out of the event loop)

but now you cannot, because every time when you try to resolve it might try to re-read the file.

@mostroverkhov
Copy link
Contributor Author

@normanmaurer It is blocking call on filesystem

while ((line = buff.readLine()) != null) {
, It comes from
final List<InetAddress> hostsFileEntries = resolveHostsFileEntries(hostname);
, so given resolveAll(..) signature I'd expect most people call It on event loop

@normanmaurer normanmaurer merged commit 904a692 into netty:4.1 Dec 23, 2021
@normanmaurer
Copy link
Member

@mostroverkhov @violetagg good point! Thanks a lot!

normanmaurer pushed a commit that referenced this pull request Dec 23, 2021
…osts by default (#11943)



Motivation:

After #11922, by default `io.netty.resolver.DefaultHostsFileEntriesResolver` synchronously reads `/etc/hosts` every 60 seconds, while before It read `/etc/hosts` once upon instantiation.

Modification:

By default `io.netty.resolver.DefaultHostsFileEntriesResolver` reads `/etc/hosts` once upon instantiation. This corresponds to io.netty.hostsFileRefreshInterval = 0

Negative values for io.netty.hostsFileRefreshInterval are forbidden

Result:

`io.netty.resolver.DefaultHostsFileEntriesResolver` reads `/etc/hosts` once upon instantiation, which corresponds to default behaviour prior to #11922
@normanmaurer normanmaurer added this to the 4.1.73.Final milestone Dec 23, 2021
@mostroverkhov mostroverkhov deleted the hosts-file-resolver branch December 30, 2021 13:15
10brothers pushed a commit to 10brothers/netty that referenced this pull request Jan 20, 2022
…osts by default (netty#11943)



Motivation:

After netty#11922, by default `io.netty.resolver.DefaultHostsFileEntriesResolver` synchronously reads `/etc/hosts` every 60 seconds, while before It read `/etc/hosts` once upon instantiation.

Modification:

By default `io.netty.resolver.DefaultHostsFileEntriesResolver` reads `/etc/hosts` once upon instantiation. This corresponds to io.netty.hostsFileRefreshInterval = 0

Negative values for io.netty.hostsFileRefreshInterval are forbidden

Result:

`io.netty.resolver.DefaultHostsFileEntriesResolver` reads `/etc/hosts` once upon instantiation, which corresponds to default behaviour prior to netty#11922
dlg99 pushed a commit to apache/bookkeeper that referenced this pull request Feb 2, 2022
### Motivation

Changelog: https://netty.io/news/2022/01/12/4-1-73-Final.html

The main reason to upgrade is because of an [intensive I/O disk scheduled task](netty/netty#11943) introduced in 4.1.72.Final which is synchronous and can cause EventLoop to blocked very often. 

### Changes

* Upgrade Netty from 4.1.72.Final to 4.1.73.Final
* [Netty 4.1.73.Final depends on netty-tc-native 2.0.46](https://github.com/netty/netty/blob/b5219aeb4ee62f15d5dfb2b9c29d0c694aca05be/pom.xml#L545) as Netty 4.1.72.Final, so no need to upgrade



Reviewers: Andrey Yegorov <None>

This closes #3020 from nicoloboschi/upgrade-netty-4.1.73
StevenLuMT pushed a commit to StevenLuMT/bookkeeper that referenced this pull request Feb 16, 2022
### Motivation

Changelog: https://netty.io/news/2022/01/12/4-1-73-Final.html

The main reason to upgrade is because of an [intensive I/O disk scheduled task](netty/netty#11943) introduced in 4.1.72.Final which is synchronous and can cause EventLoop to blocked very often. 

### Changes

* Upgrade Netty from 4.1.72.Final to 4.1.73.Final
* [Netty 4.1.73.Final depends on netty-tc-native 2.0.46](https://github.com/netty/netty/blob/b5219aeb4ee62f15d5dfb2b9c29d0c694aca05be/pom.xml#L545) as Netty 4.1.72.Final, so no need to upgrade



Reviewers: Andrey Yegorov <None>

This closes apache#3020 from nicoloboschi/upgrade-netty-4.1.73
raidyue pushed a commit to raidyue/netty that referenced this pull request Jul 8, 2022
…osts by default (netty#11943)



Motivation:

After netty#11922, by default `io.netty.resolver.DefaultHostsFileEntriesResolver` synchronously reads `/etc/hosts` every 60 seconds, while before It read `/etc/hosts` once upon instantiation.

Modification:

By default `io.netty.resolver.DefaultHostsFileEntriesResolver` reads `/etc/hosts` once upon instantiation. This corresponds to io.netty.hostsFileRefreshInterval = 0

Negative values for io.netty.hostsFileRefreshInterval are forbidden

Result:

`io.netty.resolver.DefaultHostsFileEntriesResolver` reads `/etc/hosts` once upon instantiation, which corresponds to default behaviour prior to netty#11922
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.

None yet

3 participants