From b62222c301d8bc194d9ebc67f07d0c8a34755c51 Mon Sep 17 00:00:00 2001 From: Ingo Bauersachs Date: Sat, 18 Jan 2025 17:10:04 +0100 Subject: [PATCH 1/2] Reduce hosts file parsing warnings Closes #361 --- .../org/xbill/DNS/hosts/HostsFileParser.java | 42 +++++++++++++++---- 1 file changed, 35 insertions(+), 7 deletions(-) diff --git a/src/main/java/org/xbill/DNS/hosts/HostsFileParser.java b/src/main/java/org/xbill/DNS/hosts/HostsFileParser.java index c617bc6d..e0e0a7d4 100644 --- a/src/main/java/org/xbill/DNS/hosts/HostsFileParser.java +++ b/src/main/java/org/xbill/DNS/hosts/HostsFileParser.java @@ -14,6 +14,7 @@ import java.util.Map; import java.util.Objects; import java.util.Optional; +import java.util.concurrent.atomic.AtomicInteger; import lombok.RequiredArgsConstructor; import lombok.extern.slf4j.Slf4j; import org.xbill.DNS.Address; @@ -37,6 +38,7 @@ public final class HostsFileParser { private final boolean clearCacheOnChange; private Instant lastFileReadTime = Instant.MIN; private boolean isEntireFileParsed; + private boolean hostsFileWarningLogged = false; /** * Creates a new instance based on the current OS's default. Unix and alike (or rather everything @@ -116,9 +118,11 @@ public synchronized Optional getAddressForHost(Name name, int type) private void parseEntireHostsFile() throws IOException { String line; int lineNumber = 0; + AtomicInteger addressFailures = new AtomicInteger(0); + AtomicInteger nameFailures = new AtomicInteger(0); try (BufferedReader hostsReader = Files.newBufferedReader(path, StandardCharsets.UTF_8)) { while ((line = hostsReader.readLine()) != null) { - LineData lineData = parseLine(++lineNumber, line); + LineData lineData = parseLine(++lineNumber, line, addressFailures, nameFailures); if (lineData != null) { for (Name lineName : lineData.names) { InetAddress lineAddress = @@ -129,15 +133,26 @@ private void parseEntireHostsFile() throws IOException { } } + if (!hostsFileWarningLogged && (addressFailures.get() > 0 || nameFailures.get() > 0)) { + log.warn( + "Failed to parse entire hosts file {}, address failures={}, name failures={}", + path, + addressFailures.get(), + nameFailures); + hostsFileWarningLogged = true; + } + isEntireFileParsed = true; } private void searchHostsFileForEntry(Name name, int type) throws IOException { String line; int lineNumber = 0; + AtomicInteger addressFailures = new AtomicInteger(0); + AtomicInteger nameFailures = new AtomicInteger(0); try (BufferedReader hostsReader = Files.newBufferedReader(path, StandardCharsets.UTF_8)) { while ((line = hostsReader.readLine()) != null) { - LineData lineData = parseLine(++lineNumber, line); + LineData lineData = parseLine(++lineNumber, line, addressFailures, nameFailures); if (lineData != null) { for (Name lineName : lineData.names) { boolean isSearchedEntry = lineName.equals(name); @@ -151,6 +166,16 @@ private void searchHostsFileForEntry(Name name, int type) throws IOException { } } } + + if (!hostsFileWarningLogged && (addressFailures.get() > 0 || nameFailures.get() > 0)) { + log.warn( + "Failed to find {} in hosts file {}, address failures={}, name failures={}", + name, + path, + addressFailures.get(), + nameFailures); + hostsFileWarningLogged = true; + } } @RequiredArgsConstructor @@ -160,7 +185,8 @@ private static final class LineData { final Iterable names; } - private LineData parseLine(int lineNumber, String line) { + private LineData parseLine( + int lineNumber, String line, AtomicInteger addressFailures, AtomicInteger nameFailures) { String[] lineTokens = getLineTokens(line); if (lineTokens.length < 2) { return null; @@ -174,24 +200,26 @@ private LineData parseLine(int lineNumber, String line) { } if (lineAddressBytes == null) { - log.warn("Could not decode address {}, {}#L{}", lineTokens[0], path, lineNumber); + log.debug("Could not decode address {}, {}#L{}", lineTokens[0], path, lineNumber); + addressFailures.incrementAndGet(); return null; } Iterable lineNames = Arrays.stream(lineTokens) .skip(1) - .map(lineTokenName -> safeName(lineTokenName, lineNumber)) + .map(lineTokenName -> safeName(lineTokenName, lineNumber, nameFailures)) .filter(Objects::nonNull) ::iterator; return new LineData(lineAddressType, lineAddressBytes, lineNames); } - private Name safeName(String name, int lineNumber) { + private Name safeName(String name, int lineNumber, AtomicInteger nameFailures) { try { return Name.fromString(name, Name.root); } catch (TextParseException e) { - log.warn("Could not decode name {}, {}#L{}, skipping", name, path, lineNumber); + log.debug("Could not decode name {}, {}#L{}, skipping", name, path, lineNumber); + nameFailures.incrementAndGet(); return null; } } From 60029c154b7ccf7a0fd284c210c4659728d3ca8e Mon Sep 17 00:00:00 2001 From: Ingo Bauersachs Date: Sun, 19 Jan 2025 17:49:03 +0100 Subject: [PATCH 2/2] Reduce lock contention and disk access in hosts file parsing Closes #369 --- .../org/xbill/DNS/hosts/HostsFileParser.java | 91 +++++++++++++++---- .../xbill/DNS/hosts/HostsFileParserTest.java | 17 +++- 2 files changed, 87 insertions(+), 21 deletions(-) diff --git a/src/main/java/org/xbill/DNS/hosts/HostsFileParser.java b/src/main/java/org/xbill/DNS/hosts/HostsFileParser.java index e0e0a7d4..cd5dfb88 100644 --- a/src/main/java/org/xbill/DNS/hosts/HostsFileParser.java +++ b/src/main/java/org/xbill/DNS/hosts/HostsFileParser.java @@ -8,12 +8,14 @@ import java.nio.file.Files; import java.nio.file.Path; import java.nio.file.Paths; +import java.time.Clock; +import java.time.Duration; import java.time.Instant; import java.util.Arrays; -import java.util.HashMap; import java.util.Map; import java.util.Objects; import java.util.Optional; +import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.atomic.AtomicInteger; import lombok.RequiredArgsConstructor; import lombok.extern.slf4j.Slf4j; @@ -32,13 +34,23 @@ public final class HostsFileParser { private final int maxFullCacheFileSizeBytes = Integer.parseInt(System.getProperty("dnsjava.hostsfile.max_size_bytes", "16384")); + private final Duration fileChangeCheckInterval = + Duration.ofMillis( + Integer.parseInt( + System.getProperty("dnsjava.hostsfile.change_check_interval_ms", "300000"))); - private final Map hostsCache = new HashMap<>(); private final Path path; private final boolean clearCacheOnChange; + private Clock clock = Clock.systemUTC(); + + @SuppressWarnings("java:S3077") + private volatile Map hostsCache; + + private Instant lastFileModificationCheckTime = Instant.MIN; private Instant lastFileReadTime = Instant.MIN; private boolean isEntireFileParsed; private boolean hostsFileWarningLogged = false; + private long hostsFileSizeBytes; /** * Creates a new instance based on the current OS's default. Unix and alike (or rather everything @@ -88,8 +100,7 @@ public HostsFileParser(Path path, boolean clearCacheOnChange) { * @throws IllegalArgumentException when {@code type} is not {@link org.xbill.DNS.Type#A} or{@link * org.xbill.DNS.Type#AAAA}. */ - public synchronized Optional getAddressForHost(Name name, int type) - throws IOException { + public Optional getAddressForHost(Name name, int type) throws IOException { Objects.requireNonNull(name, "name is required"); if (type != Type.A && type != Type.AAAA) { throw new IllegalArgumentException("type can only be A or AAAA"); @@ -102,13 +113,11 @@ public synchronized Optional getAddressForHost(Name name, int type) return Optional.of(cachedAddress); } - if (isEntireFileParsed || !Files.exists(path)) { + if (isEntireFileParsed) { return Optional.empty(); } - if (Files.size(path) <= maxFullCacheFileSizeBytes) { - parseEntireHostsFile(); - } else { + if (hostsFileSizeBytes > maxFullCacheFileSizeBytes) { searchHostsFileForEntry(name, type); } @@ -141,8 +150,6 @@ private void parseEntireHostsFile() throws IOException { nameFailures); hostsFileWarningLogged = true; } - - isEntireFileParsed = true; } private void searchHostsFileForEntry(Name name, int type) throws IOException { @@ -235,21 +242,61 @@ private String[] getLineTokens(String line) { } private void validateCache() throws IOException { - if (clearCacheOnChange) { + if (!clearCacheOnChange) { + if (hostsCache == null) { + synchronized (this) { + if (hostsCache == null) { + readHostsFile(); + } + } + } + + return; + } + + if (lastFileModificationCheckTime.plus(fileChangeCheckInterval).isBefore(clock.instant())) { + log.debug("Checked for changes more than 5minutes ago, checking"); // A filewatcher / inotify etc. would be nicer, but doesn't work. c.f. the write up at // https://blog.arkey.fr/2019/09/13/watchservice-and-bind-mount/ - Instant fileTime = - Files.exists(path) ? Files.getLastModifiedTime(path).toInstant() : Instant.MAX; - if (fileTime.isAfter(lastFileReadTime)) { - // skip logging noise when the cache is empty anyway - if (!hostsCache.isEmpty()) { - log.info("Local hosts database has changed at {}, clearing cache", fileTime); - hostsCache.clear(); + + synchronized (this) { + if (!lastFileModificationCheckTime + .plus(fileChangeCheckInterval) + .isBefore(clock.instant())) { + log.debug("Never mind, check fulfilled in another thread"); + return; + } + + lastFileModificationCheckTime = clock.instant(); + readHostsFile(); + } + } + } + + private void readHostsFile() throws IOException { + if (Files.exists(path)) { + Instant fileTime = Files.getLastModifiedTime(path).toInstant(); + if (!lastFileReadTime.equals(fileTime)) { + createOrClearCache(); + + hostsFileSizeBytes = Files.size(path); + if (hostsFileSizeBytes <= maxFullCacheFileSizeBytes) { + parseEntireHostsFile(); + isEntireFileParsed = true; } - isEntireFileParsed = false; lastFileReadTime = fileTime; } + } else { + createOrClearCache(); + } + } + + private void createOrClearCache() { + if (hostsCache == null) { + hostsCache = new ConcurrentHashMap<>(); + } else { + hostsCache.clear(); } } @@ -259,6 +306,10 @@ private String key(Name name, int type) { // for unit testing only int cacheSize() { - return hostsCache.size(); + return hostsCache == null ? 0 : hostsCache.size(); + } + + void setClock(Clock clock) { + this.clock = clock; } } diff --git a/src/test/java/org/xbill/DNS/hosts/HostsFileParserTest.java b/src/test/java/org/xbill/DNS/hosts/HostsFileParserTest.java index 921e0beb..20700a9b 100644 --- a/src/test/java/org/xbill/DNS/hosts/HostsFileParserTest.java +++ b/src/test/java/org/xbill/DNS/hosts/HostsFileParserTest.java @@ -5,6 +5,8 @@ import static org.junit.jupiter.api.Assertions.assertNotEquals; import static org.junit.jupiter.api.Assertions.assertThrows; import static org.junit.jupiter.api.Assertions.assertTrue; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; import java.io.BufferedWriter; import java.io.IOException; @@ -18,6 +20,9 @@ import java.nio.file.StandardCopyOption; import java.nio.file.StandardOpenOption; import java.nio.file.attribute.FileTime; +import java.time.Clock; +import java.time.Duration; +import java.time.Instant; import java.util.Optional; import org.junit.jupiter.api.BeforeAll; import org.junit.jupiter.api.Test; @@ -72,7 +77,7 @@ void testMissingFileIsEmptyResult() throws IOException { } @Test - void testCacheLookup() throws IOException { + void testCacheLookupAfterFileDeleteWithoutChangeChecking() throws IOException { Path tempHosts = Files.copy(hostsFileWindows, tempDir, StandardCopyOption.REPLACE_EXISTING); HostsFileParser hostsFileParser = new HostsFileParser(tempHosts, false); assertEquals(0, hostsFileParser.cacheSize()); @@ -98,6 +103,10 @@ void testFileDeletionClearsCache() throws IOException { tempDir.resolve("testFileWatcherClearsCache"), StandardCopyOption.REPLACE_EXISTING); HostsFileParser hostsFileParser = new HostsFileParser(tempHosts); + Clock clock = mock(Clock.class); + hostsFileParser.setClock(clock); + Instant now = Clock.systemUTC().instant(); + when(clock.instant()).thenReturn(now); assertEquals(0, hostsFileParser.cacheSize()); assertEquals( kubernetesAddress, @@ -106,6 +115,7 @@ void testFileDeletionClearsCache() throws IOException { .orElseThrow(() -> new IllegalStateException("Host entry not found"))); assertTrue(hostsFileParser.cacheSize() > 1, "Cache must not be empty"); Files.delete(tempHosts); + when(clock.instant()).thenReturn(now.plus(Duration.ofMinutes(6))); assertEquals(Optional.empty(), hostsFileParser.getAddressForHost(kubernetesName, Type.A)); assertEquals(0, hostsFileParser.cacheSize()); } @@ -119,6 +129,10 @@ void testFileChangeClearsCache() throws IOException { StandardCopyOption.REPLACE_EXISTING); Files.setLastModifiedTime(tempHosts, FileTime.fromMillis(0)); HostsFileParser hostsFileParser = new HostsFileParser(tempHosts); + Clock clock = mock(Clock.class); + hostsFileParser.setClock(clock); + Instant now = Clock.systemUTC().instant(); + when(clock.instant()).thenReturn(now); assertEquals(0, hostsFileParser.cacheSize()); assertEquals( kubernetesAddress, @@ -134,6 +148,7 @@ void testFileChangeClearsCache() throws IOException { } Files.setLastModifiedTime(tempHosts, FileTime.fromMillis(10_0000)); + when(clock.instant()).thenReturn(now.plus(Duration.ofMinutes(6))); assertEquals( InetAddress.getByAddress(testName.toString(), localhostBytes), hostsFileParser