feat(nameresolving): add system resolver worker pool and factory by richard-ramos · Pull Request #3384 · iftech/nim-libp2p

richard-ramos · 2026-11-04T13:36:57Z

Summary

Resolve names through the host's OS configuration, including hosts files, localhost, and container DNS. Blocking lookups run on a lazily created worker pool; TXT queries use the system DNS servers discovered by the parent PR.

Add newNameResolver: omitting the server list selects SystemResolver; supplying a list selects DnsResolver, including an empty list that returns no DNS results. Cover family filtering, concurrent lookups, cancellation, asynchronous shutdown, concurrent stops, reopening, and real switch integration.

Impact on Library Users

Import libp2p/nameresolving. newNameResolver(rng) returns a SystemResolver. The caller must await its stop() after stopping dependent switches. Workers start lazily on first use; start() permits reuse after shutdown.

newNameResolver(rng, servers) returns a DnsResolver, which requires no lifecycle calls. Passing either resolver to withNameResolver leaves ownership with the caller. A TXT resolver supplied to SystemResolver.new also remains caller-managed: the system resolver does not start or stop it.

The system resolver requires threads, and shutdown waits asynchronously for active OS lookups. Unless a TXT resolver is supplied, construction discovers TXT nameservers synchronously and can block the calling thread while reading system configuration.

Stack

2 of 3 — merge in this order:

#3379: system DNS discovery; base master.
This PR: system resolver worker pool and factory; base feat/system-name-resolver.
#3385: bounded multiaddress expansion; base feat/system-resolver-backend.

All three PRs are drafts. Review this diff against its parent branch.

Validation

System-resolver suite, including caller-managed lifecycle and real switch DNS dialing: 13 checks passed after rebasing.

References

Closes #3378. Continues the resolver implementation from #3114, originally authored by @nate-staked.

richard-ramos · 2026-11-04T13:49:33Z

/review

github-actions · 2026-11-04T14:07:59Z

🔍 OpenCodeReview found 2 issue(s) in this PR.

✅ Successfully posted inline: 2 comment(s)

Model: qwen3.8-max
Review generated in: 18m 25s

vladopajic · 2026-11-06T12:01:41Z

+    checkTrackers()
+
+  asyncTest "switch resolves localhost with a caller-managed system resolver":
+    let resolver = newNameResolver(rng())


should be
let resolver = SystemResolver.new(rng)
in test files here, since they are testing this type.

newNameResolver() should be used only in test that test libp2p/nameresolving.nim file

vladopajic · 2026-11-06T12:10:21Z

@@ -0,0 +1,359 @@
+# SPDX-License-Identifier: Apache-2.0 OR MIT


filename system_resolver.nim

vladopajic · 2026-11-06T12:12:16Z

+const
+  MaxSystemAddresses = 100
+  IPv4MappedMarkerByte = 0xff'u8
+


most of methods in this file seem to raise instead of returning LPResult - which should be norm in this repo. is intentional?

vladopajic · 2026-11-06T12:15:30Z

+
+proc newNameResolver*(rng: Rng, nameServers: seq[TransportAddress]): DnsResolver =
+  ## Use only the supplied DNS servers. An empty list produces no DNS results.
+  DnsResolver.new(nameServers, rng)
+
+proc newNameResolver*(rng: Rng): SystemResolver =
+  ## Resolve IPs through the OS. Discovers TXT nameservers synchronously during
+  ## construction, which can block the calling thread while reading system config.
+  ## Requires threads. The caller must await stop() to release its workers.
+  SystemResolver.new(rng)


seems unnecessary, its more descriptive and nicer if downstream users use exactly the type they would like, becasue newNameResolver actually hides what implementation is used. even in our code in libary is better to use Type.new:

DnsResolver.new(nameServers, rng)
or
SystemResolver.new(rng)

nit DnsResolver.new(rng, nameServers) (rng as first arg if possible)

vladopajic · 2026-11-06T12:27:08Z

@@ -0,0 +1,182 @@
+# SPDX-License-Identifier: Apache-2.0 OR MIT
+# Copyright (c) Status Research & Development GmbH


tests need better organization:

test that test SystemResolver should be in test_system_resolve file

test that test DnsResolver should be in test_dns_resolver file

both of above should/must not use Switch for testing. they should be 100% pure type unit tests. and they should cover all invariant of these types.

since they are testing same interface NameResolver there should be suite_name_resolver that will assert basic invariant of any NameResolver (similarly to transport testes, KadStore tests, AutoTLS state tests)

test_system_resolve = suite_name_resolver + all specific tests for SystemResolver (if any, but i guess there is)
test_dns_resolver = suite_name_resolver + all specific tests for DnsResolver (if any)
^
both of above will just factor method to pass concrete NameResolver implementation to suite_name_resolver.

with all tests from above there is not need to extensively use Switch tests, as all invariant of these types are full testes in isolation.

there could be tests that test integration of NameResolver + other components in Switch. but those should only cover that integration parts, it should not cover part that test if NameResolver are working as expected because those test are already tested in tests above.

vladopajic · 2026-11-06T12:28:21Z

+
+import std/[sequtils, strutils, tables]
+import chronos, chronos/threadsync
+import ../../libp2p/nameresolving


in theory test_{filename}.nim should only import and test libp2p2/{filename}.nim type. it should be unit test for that type.

vladopajic · 2026-11-06T12:46:55Z

+    requestSignal, responseSignal: ThreadSignalPtr
+    when defined(libp2p_testing):
+      workerEntered, workerRelease: ThreadSignalPtr
+      workerGateReleased: Atomic[bool]


initially usage of libp2p_testing was envisioned only for exporting. because conditional logic can have behavior that may lead to unexpected outcomes when flag libp2p_testing is not used. note that tests only test when libp2p_testing flag is used.

in order to avoid such complexity, libp2p_testing was started to be used as export only, because worst thing that can happen (to downstream) is there is just more exported things.

note that this very similar principle as conditional feature flags, which we already agreed no to use.

is it really necessary to have this in tests?

if yes, could there be some new abstraction "new logic" that could be tested in isolation (for example running and maintaining workers and worker loop). so that responsibility of SystemResolver is minimized only to providing and describing needed work, to that "new logic" that drives loop, maintains workers, synchronizes, etc..

ideally it would be nicer to have logic specific to system_resolver here, anything related to managing threads and loop should be in different type. because when someone is looking at this file they care about what system_resolver does, and now how thread pool is managed. the later abstraction could potentially be used in other other components.

vladopajic · 2026-11-06T12:48:05Z

+      workerEntered, workerRelease: ThreadSignalPtr
+      workerGateReleased: Atomic[bool]
+
+  PendingRequest = object


could async-channels be used here to avoid/minimize shared state?
https://github.com/status-im/nim-async-channels/

gmelodie · 2026-11-06T13:44:07Z

+proc isV4Mapped(address: TransportAddress): bool =
+  if address.family != AddressFamily.IPv6:
+    return false
+  for i in 0 ..< 10:
+    if address.address_v6[i] != 0:
+      return false
+  address.address_v6[10] == IPv4MappedMarkerByte and
+    address.address_v6[11] == IPv4MappedMarkerByte


I think we could use isIPv6/4 from utils/ipaddr.nim, or add these helpers there

gmelodie · 2026-11-06T13:57:55Z

+            let workerRelease = shared[].workerRelease
+            if not workerRelease.isNil:
+              shared[].workerRelease = nil
+              discard shared[].workerEntered.fireSync()
+              let waitResult = workerRelease.waitSync(2.seconds)
+              shared[].workerGateReleased.store(waitResult.isOk and waitResult.get())


Extract this into a function to make it easier to read

gmelodie · 2026-11-06T14:01:15Z

+              if req.domain == Domain.AF_INET6 and address.isV4Mapped():
+                continue


Can the reverse not happen? i.e. domain.AF_INET4 and address.isV6mapped

gmelodie · 2026-11-06T14:01:54Z

+              if req.resultsLen == MaxSystemAddresses:
+                break


why not

Suggested change

if req.resultsLen == MaxSystemAddresses:

break

if req.resultsLen >= MaxSystemAddresses:

break

also maybe log this? also now looking at it why is this not tested before the loop is even running?

gmelodie · 2026-11-06T14:04:19Z

+        shared[].lock.acquire()
+        req.next = shared[].completedHead
+        shared[].completedHead = req
+        shared[].lock.release()


can we use withLock here? other places too

gmelodie · 2026-11-06T14:12:31Z

+    deinitLock(shared[].lock)
+    deallocShared(shared)
+    raise newException(
+      TransportAddressError, "Failed to spawn resolver worker: " & exc.msg, exc
+    )


This pattern exists 4 times in the code, maybe do

proc destroy(shared: ptr SharedState) = ## Release every resource `shared` owns. Safe on a partially initialized state. if not shared[].requestSignal.isNil: discard shared[].requestSignal.close() if not shared[].responseSignal.isNil: discard shared[].responseSignal.close() deinitLock(shared[].lock) deallocShared(shared)

then use it as

shared[].requestSignal = ThreadSignalPtr.new().valueOr: shared.destroy() raise newException(TransportAddressError, "Failed to create request signal: " & error) shared[].responseSignal = ThreadSignalPtr.new().valueOr: shared.destroy() raise newException(TransportAddressError, "Failed to create response signal: " & error) ... except ResourceExhaustedError as exc: workers.setLen(started) shared[].running.store(false) discard shared[].requestSignal.fireSync() joinThreads(workers) shared.destroy() raise newException(TransportAddressError, "Failed to spawn resolver worker: " & exc.msg, exc)

gmelodie · 2026-11-06T14:13:22Z

+): Future[seq[TransportAddress]] {.
+    async: (raises: [CancelledError, TransportAddressError])
+.} =
+  if self.stopped or address.len == 0 or address.len > 255 or '\0' in address:


Suggested change

if self.stopped or address.len == 0 or address.len > 255 or '\0' in address:

if self.stopped or not address.isValid():

gmelodie · 2026-11-06T14:15:52Z

+    var expected = ord(RequestState.Pending)
+    discard req.state.compareExchange(expected, ord(RequestState.Discarded))


Huh... can this be a settle template?

proc settle(req: ptr SharedRequest, state: RequestState): bool {.discardable.} = ## Move `req` from Pending to `state`. False if it already left Pending. var expected = ord(RequestState.Pending) req.state.compareExchange(expected, ord(state))

Suggested change

var expected = ord(RequestState.Pending)

discard req.state.compareExchange(expected, ord(RequestState.Discarded))

proc cancellation(udata: pointer) {.gcsafe, raises: [].} =

req.settle(RequestState.Discarded)

Can be used in the worker loop too

req.settle(RequestState.Done) except TransportAddressError: req.settle(RequestState.Discarded)

gmelodie · 2026-11-06T14:16:07Z

+  fut.cancelCallback = cancellation
+  self.pending.add(PendingRequest(req: req, fut: fut))
+
+  shared[].lock.acquire()


gmelodie · 2026-11-06T14:16:20Z

+    return
+
+  let shared = self.shared
+  shared[].lock.acquire()


github-project-automation Bot added this to nim-libp2p Nov 4, 2026

github-project-automation Bot moved this to new in nim-libp2p Nov 4, 2026

github-actions Bot assigned richard-ramos Nov 4, 2026

This was referenced Nov 4, 2026

feat(nameresolving): bound and expand composed DNS multiaddresses #3385

Draft

feat(nameresolving): add system DNS discovery #3379

Open

richard-ramos added this pull request to stack #3386 November 4, 2026 13:41

richard-ramos force-pushed the feat/system-resolver-backend branch from 95975d2 to b11fb8e Compare November 4, 2026 13:44

github-actions Bot reviewed Nov 4, 2026

View reviewed changes

Comment thread libp2p/nameresolving.nim Resolved

Comment thread libp2p/nameresolving.nim Resolved

richard-ramos force-pushed the feat/system-resolver-backend branch 2 times, most recently from 4df7d94 to 7aa78f4 Compare November 5, 2026 22:57

richard-ramos added 5 commits November 5, 2026 19:06

feat(nameresolving): add system resolver worker pool and factory

227c883

refactor(nameresolving): name pending resolver requests

00740bc

refactor(nameresolving): name IPv4-mapped marker byte

b2701d4

fix(nameresolving): distinguish omitted and empty DNS servers

4870898

refactor(nameresolving): keep system resolver lifecycle caller-managed

c06c767

richard-ramos force-pushed the feat/system-resolver-backend branch from 7aa78f4 to c06c767 Compare November 5, 2026 23:07

richard-ramos marked this pull request as ready for review November 5, 2026 23:10

richard-ramos requested review from a team, gmelodie and vladopajic November 5, 2026 23:10

vladopajic approved these changes Nov 6, 2026

View reviewed changes

gmelodie approved these changes Nov 6, 2026

View reviewed changes

		@@ -0,0 +1,359 @@
		# SPDX-License-Identifier: Apache-2.0 OR MIT

		@@ -0,0 +1,182 @@
		# SPDX-License-Identifier: Apache-2.0 OR MIT
		# Copyright (c) Status Research & Development GmbH

		if req.domain == Domain.AF_INET6 and address.isV4Mapped():
		continue

	if self.stopped or address.len == 0 or address.len > 255 or '\0' in address:
	if self.stopped or not address.isValid():

		var expected = ord(RequestState.Pending)
		discard req.state.compareExchange(expected, ord(RequestState.Discarded))

Conversation

richard-ramos commented Nov 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Impact on Library Users

Stack

Validation

References

Uh oh!

richard-ramos commented Nov 4, 2026

Uh oh!

github-actions Bot commented Nov 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This branch has not been deployed

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

richard-ramos commented Nov 4, 2026 •

edited

Loading

github-actions Bot commented Nov 4, 2026 •

edited

Loading