Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 34 additions & 0 deletions libp2p/nameresolving.nim
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
# SPDX-License-Identifier: Apache-2.0 OR MIT
# Copyright (c) Status Research & Development GmbH

## Choose a DNS resolver with explicit servers or an OS-backed resolver:
##
## ```nim
## let resolver = newNameResolver(rng)
## let addresses = await resolver.resolveMAddress(address)
## await resolver.stop()
## ```
##
## Passing a resolver to `SwitchBuilder.withNameResolver` keeps its lifecycle
## caller-managed. Stop dependent switches before awaiting `SystemResolver.stop()`.
##
## The OS resolver requires threads. Its TXT queries use discovered system DNS
## servers, with DefaultDnsServers as a fallback when discovery finds none.

{.push raises: [].}

import chronos
import ./nameresolving/[nameresolver, dnsresolver, systemresolver]
import ./crypto/rng

export nameresolver, dnsresolver, systemresolver

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)
Comment on lines +25 to +34

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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)

359 changes: 359 additions & 0 deletions libp2p/nameresolving/systemresolver.nim
Original file line number Diff line number Diff line change
@@ -0,0 +1,359 @@
# SPDX-License-Identifier: Apache-2.0 OR MIT

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

filename system_resolver.nim

# Copyright (c) Status Research & Development GmbH

## Resolve IPs through the OS, including hosts files and search domains.
## Blocking getaddrinfo calls run on worker threads; TXT queries use a fallback
## NameResolver. Call stop() to release workers, and start() to reuse a resolver.

{.push raises: [].}

import std/[atomics, locks]
import chronos, chronos/threadsync, chronicles
import nameresolver, dnsresolver
import ../crypto/rng

logScope:
topics = "libp2p systemresolver"

const
MaxSystemAddresses = 100
IPv4MappedMarkerByte = 0xff'u8

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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

type
RequestState {.pure.} = enum
Pending
Done
Discarded

# Everything the worker threads touch lives in the shared heap and is
# plain data - no GC'd Nim types may cross the thread boundary.
SharedRequest = object
next: ptr SharedRequest
host: array[256, char] # null-terminated, copied at submission
port: uint16
domain: Domain
state: Atomic[int] # RequestState
results: array[MaxSystemAddresses, TransportAddress]
resultsLen: int

SharedState = object
lock: Lock
queueHead, queueTail: ptr SharedRequest
completedHead: ptr SharedRequest # singly-linked via `next`
running: Atomic[bool]
workersRunning: Atomic[int]
requestSignal, responseSignal: ThreadSignalPtr
when defined(libp2p_testing):
workerEntered, workerRelease: ThreadSignalPtr
workerGateReleased: Atomic[bool]

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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.

  1. is it really necessary to have this in tests?
  2. 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..

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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.


PendingRequest = object

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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

req: ptr SharedRequest
fut: Future[seq[TransportAddress]].Raising([CancelledError])

SystemResolver* = ref object of NameResolver
## Resolves names through the OS resolver (getaddrinfo), offloaded to
## lazily-created worker threads.
txtResolver: NameResolver
shared: ptr SharedState
workers: seq[Thread[ptr SharedState]]
workerCount: int
pending: seq[PendingRequest]
dispatcherFut: Future[void].Raising([])
stopFut: Future[void].Raising([])
stopped: bool
when defined(libp2p_testing):
workerEntered, workerRelease: ThreadSignalPtr
workerGateReleased: bool

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
Comment on lines +69 to +76

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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


proc workerLoop(shared: ptr SharedState) {.thread.} =
{.cast(gcsafe).}:
while shared[].running.load():
discard shared[].requestSignal.waitSync()
while true:
var req: ptr SharedRequest
shared[].lock.acquire()
if shared[].running.load():
req = shared[].queueHead
if not req.isNil:
shared[].queueHead = req.next
if shared[].queueHead.isNil:
shared[].queueTail = nil
shared[].lock.release()
if req.isNil:
break

if RequestState(req.state.load()) == RequestState.Pending:
let host = $cast[cstring](addr req.host[0])
when defined(libp2p_testing):
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())
Comment on lines +98 to +103

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Extract this into a function to make it easier to read

try:
let resolved = resolveTAddress(host, Port(req.port), req.domain)
for address in resolved:
# getaddrinfo can return mapped literals even without AI_V4MAPPED.
if req.domain == Domain.AF_INET6 and address.isV4Mapped():
continue
Comment on lines +108 to +109

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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

if req.resultsLen == MaxSystemAddresses:
break
Comment on lines +110 to +111

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

why not

Suggested change
if req.resultsLen == MaxSystemAddresses:
break
if req.resultsLen >= MaxSystemAddresses:
break

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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

req.results[req.resultsLen] = address
inc req.resultsLen
var expected = ord(RequestState.Pending)
discard req.state.compareExchange(expected, ord(RequestState.Done))
except TransportAddressError:
var expected = ord(RequestState.Pending)
discard req.state.compareExchange(expected, ord(RequestState.Discarded))

shared[].lock.acquire()
req.next = shared[].completedHead
shared[].completedHead = req
shared[].lock.release()
Comment on lines +120 to +123

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

can we use withLock here? other places too

discard shared[].responseSignal.fireSync()

# Wake another idle worker during shutdown, then notify the event-loop
# thread that this worker no longer touches shared state.
discard shared[].requestSignal.fireSync()
discard shared[].workersRunning.fetchSub(1)
discard shared[].responseSignal.fireSync()

proc finishRequest(self: SystemResolver, req: ptr SharedRequest) =
## Complete (or skip, when cancelled) the future waiting on `req` and
## free the shared request. Event-loop thread only.
var idx = -1
for i, p in self.pending:
if p.req == req:
idx = i
break
Comment on lines +135 to +139

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

maybe create a template

template findIt(s, pred: untyped): Opt[int] =
  var res = Opt.none(int)
  for i, it {.inject.} in s:
    if pred:
      res = Opt.some(i)
      break
  res

then

self.pending.findIt(it.req == req).ifValue(idx):
  let fut = self.pending[idx].fut
  self.pending.del(idx)
  if not fut.finished():
    ...
    fut.complete(res)
deallocShared(req)

if idx >= 0:
let fut = self.pending[idx].fut
self.pending.del(idx)
if not fut.finished():
let deliver = RequestState(req.state.load()) == RequestState.Done
var res = newSeqOfCap[TransportAddress](if deliver: req.resultsLen else: 0)
if deliver:
for i in 0 ..< req.resultsLen:
res.add(req.results[i])
fut.complete(res)
Comment on lines +143 to +149

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Suggested change
if not fut.finished():
let deliver = RequestState(req.state.load()) == RequestState.Done
var res = newSeqOfCap[TransportAddress](if deliver: req.resultsLen else: 0)
if deliver:
for i in 0 ..< req.resultsLen:
res.add(req.results[i])
fut.complete(res)
if not fut.finished():
let res =
if RequestState(req.state.load()) == RequestState.Done:
@(req.results.toOpenArray(0, req.resultsLen - 1))
else:
@[]
fut.complete(res)

deallocShared(req)

proc drainCompleted(self: SystemResolver) =
let shared = self.shared
shared[].lock.acquire()
var cur = shared[].completedHead
shared[].completedHead = nil
shared[].lock.release()
Comment on lines +154 to +157

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

withLock?

while not cur.isNil:
let req = cur
cur = req.next
self.finishRequest(req)

proc dispatchLoop(self: SystemResolver) {.async: (raises: []).} =
while not self.stopped:
try:
await self.shared[].responseSignal.wait()
except AsyncError, CancelledError:
continue
self.drainCompleted()

proc startWorkers(self: SystemResolver) {.raises: [TransportAddressError].} =
# Allocate workers lazily so unused resolvers consume no threads.
if not self.shared.isNil:
return

let shared = cast[ptr SharedState](allocShared0(sizeof(SharedState)))
initLock(shared[].lock)
shared[].running.store(true)
shared[].workersRunning.store(0)
when defined(libp2p_testing):
shared[].workerEntered = self.workerEntered
shared[].workerRelease = self.workerRelease
shared[].workerGateReleased.store(false)
shared[].requestSignal = ThreadSignalPtr.new().valueOr:
deinitLock(shared[].lock)
deallocShared(shared)
raise
newException(TransportAddressError, "Failed to create request signal: " & error)
shared[].responseSignal = ThreadSignalPtr.new().valueOr:
discard shared[].requestSignal.close()
deinitLock(shared[].lock)
deallocShared(shared)
raise
newException(TransportAddressError, "Failed to create response signal: " & error)

# createThread passes the address of the Thread object itself to pthread, so
# every object must already occupy its final, stable sequence slot.
var workers = newSeq[Thread[ptr SharedState]](self.workerCount)
var started = 0
try:
for i in 0 ..< workers.len:
createThread(workers[i], workerLoop, shared)
discard shared[].workersRunning.fetchAdd(1)
inc started
except ResourceExhaustedError as exc:
workers.setLen(started)
shared[].running.store(false)
discard shared[].requestSignal.fireSync()
joinThreads(workers)
discard shared[].requestSignal.close()
discard shared[].responseSignal.close()
deinitLock(shared[].lock)
deallocShared(shared)
raise newException(
TransportAddressError, "Failed to spawn resolver worker: " & exc.msg, exc
)
Comment on lines +212 to +216

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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)


self.shared = shared
self.workers = workers
self.dispatcherFut = self.dispatchLoop()

proc new*(
T: type SystemResolver, rng: Rng, txtResolver: NameResolver = nil, workers: int = 2
): T =
## Create a SystemResolver which starts `workers` worker threads on first use.
## `txtResolver` handles TXT queries (getaddrinfo cannot); when nil, a
## `DnsResolver` over the system nameservers is used. Discovery is synchronous
## during construction and can block the calling thread while reading system config.
doAssert workers >= 1

T(
txtResolver:
if txtResolver.isNil:
NameResolver(DnsResolver.new(getSystemNameServers(), rng))
else:
txtResolver,
workerCount: workers,
)

when defined(libp2p_testing):
proc setWorkerTestGate*(self: SystemResolver, entered, release: ThreadSignalPtr) =
## Delay the first worker lookup so shutdown behavior can be tested.
doAssert self.shared.isNil
doAssert not entered.isNil and not release.isNil
self.workerEntered = entered
self.workerRelease = release
self.workerGateReleased = false

proc workerTestGateReleased*(self: SystemResolver): bool =
self.workerGateReleased

method resolveIp*(
self: SystemResolver, address: string, port: Port, domain: Domain = Domain.AF_UNSPEC
): Future[seq[TransportAddress]] {.
async: (raises: [CancelledError, TransportAddressError])
.} =
if self.stopped or address.len == 0 or address.len > 255 or '\0' in address:

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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

return @[]

if self.shared.isNil:
self.startWorkers()

let
shared = self.shared
req = cast[ptr SharedRequest](allocShared0(sizeof(SharedRequest)))
fut = Future[seq[TransportAddress]].Raising([CancelledError]).init(
"systemresolver.resolveIp"
)
copyMem(addr req.host[0], address.cstring, address.len)
req.port = uint16(port)
req.domain = domain
req.state.store(ord(RequestState.Pending))

proc cancellation(udata: pointer) {.gcsafe, raises: [].} =
var expected = ord(RequestState.Pending)
discard req.state.compareExchange(expected, ord(RequestState.Discarded))
Comment on lines +275 to +276

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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)


fut.cancelCallback = cancellation
self.pending.add(PendingRequest(req: req, fut: fut))

shared[].lock.acquire()

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

withLock

if shared[].queueTail.isNil:
shared[].queueHead = req
else:
shared[].queueTail.next = req
shared[].queueTail = req
shared[].lock.release()
discard shared[].requestSignal.fireSync()

trace "Resolving via getaddrinfo", address, port = uint16(port), domain
return await fut

method resolveTxt*(
self: SystemResolver, address: string
): Future[seq[string]] {.async: (raises: [CancelledError]).} =
if self.stopped:
return @[]
return await self.txtResolver.resolveTxt(address)

proc start*(self: SystemResolver) {.gcsafe.} =
## Worker resources are created lazily by the next IP resolution.
if self.shared.isNil:
self.stopped = false
self.stopFut = nil

proc stopWorkers(self: SystemResolver) {.async: (raises: []).} =
self.stopped = true

if self.shared.isNil:
return

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

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

withLock

shared[].running.store(false)
shared[].lock.release()
discard shared[].requestSignal.fireSync()

# Stop the dispatcher before using its signal to await worker exits.
if not self.dispatcherFut.isNil:
discard shared[].responseSignal.fireSync()
await noCancel self.dispatcherFut
self.dispatcherFut = nil

# A worker may still be inside getaddrinfo, but waiting for it must not
# block the event loop. Every worker signals after decrementing this count.
while shared[].workersRunning.load() > 0:
try:
await shared[].responseSignal.wait()
except AsyncError, CancelledError:
continue
self.drainCompleted()

# All workers have exited, so joining only releases their thread handles.
joinThreads(self.workers)
when defined(libp2p_testing):
self.workerGateReleased = shared[].workerGateReleased.load()

# No worker can touch the shared state anymore: complete whatever is
# left, with results where the worker managed to produce them.
self.drainCompleted()
var cur = shared[].queueHead
while not cur.isNil:
let req = cur
cur = req.next
self.finishRequest(req)
Comment on lines +342 to +345

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Why is this done again here? drainCompleted has the exact same code?

 while not cur.isNil:
    let req = cur
    cur = req.next
    self.finishRequest(req)


discard shared[].requestSignal.close()
discard shared[].responseSignal.close()
deinitLock(shared[].lock)
deallocShared(shared)
self.shared = nil
self.workers = @[]

proc stop*(self: SystemResolver) {.async: (raises: []).} =
## Wait for active OS lookups, discard queued lookups, and release workers.
## Concurrent callers await the same cleanup; the event loop stays responsive.
if self.stopFut.isNil:
self.stopFut = self.stopWorkers()
await noCancel self.stopFut
Loading
Loading