Skip to content

Fix the thread safety in the dns_dispatch unit

Evan Hunt requested to merge 3178-dispatch-race into main

The dispatches are not thread-bound, and used freely between various threads (see the dns_resolver and dns_request units for details).

This refactoring make sure that all non-const dns_dispatch_t and dns_dispentry_t members are accessed under a lock, and both object now track their internal state (NONE, CONNECTING, CONNECTING, CANCELED) instead of guessing the state from the state of various struct members.

During the refactoring, the artificial limit DNS_DISPATCH_SOCKSQUOTA on UDP sockets per dispatch was removed as the limiting needs to happen and happens on in dns_resolver and limiting the number of UDP sockets artificially in dispatch could lead to unpredictable behaviour in case one dispatch has the limit exhausted by others are idle.

The TCP artificial limit of DNS_DISPATCH_MAXREQUESTS makes even less sense as the TCP connections are only reused in the dns_request API that's not a heavy user of the outgoing connections.

As a side note, the fact that UDP and TCP dispatch pretends to be same thing, but in fact the connected UDP is handled from dns_dispentry_t and dns_dispatch_t acts as a broker, but connected TCP is handled from dns_dispatch_t and dns_dispatchmgr_t acts as a broker doesn't really help the clarity of this unit.

This refactoring kept to API almost same - only dns_dispatch_cancel() and dns_dispatch_done() were merged into dns_dispatch_done() as we need to cancel active netmgr handles in any case to not leave dangling connections around. The functions handling UDP and TCP have been mostly split to their matching counterparts and the dns_dispatch_ functions are now thing wrappers that call <udp|tcp>dispatch based on the socket type.

More debugging-level logging was added to the unit to accomodate for this fact.

Closes #3178 (closed)

Edited by Ondřej Surý

Merge request reports