lightninglabs / neutrino

Privacy-Preserving Bitcoin Light Client
MIT License
900 stars 183 forks source link

chain service: defer WaitGroup Done in peerHandler #232

Closed chappjc closed 2 years ago

chappjc commented 2 years ago

The (*ChainService).peerHandler should only call s.wg.Done when it is really done, which includes use of the logger. Put the Done call at the top of the defer stack, which is generally good practice and consistent with the rest of the neutrino code base.

As far as impact of the resolved issue, consumers that wait on ChainService shutdown before tearing down the logger backend rely on ChainService actually being done with the logger. Another possible reason is that certain use cases might want to set a new log backend after closing up neutrino and before making a new instance, and doing so would be a race unless it is really done with the logger. We've encountered this issue in both scenarios.

chappjc commented 2 years ago

Turns out there's a whole bunch of these in btcd, plus a handful of completely unsupervised goroutines in btcd that do logging (see Peer). As such, resetting the logger even after shutting things down as cleanly as possible appears to be out of the question. This change is still good to ensure neutrino itself is done logging before the consumer closes up, but I'm closing the PR since the impact is low and I'm moving in a different direction to work around the issues we hit with logging.

Sorry for the PR noise.