Bug/rescan datarace #820
Closed
Bug/rescan datarace #820
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
MStreet3
force-pushed
the
bug/rescan-datarace
branch
5 times, most recently
from
05658d0 to
00ea81b
Compare
MStreet3
force-pushed
the
bug/rescan-datarace
branch
from
5edb8ab to
acea149
Compare
MStreet3
force-pushed
the
bug/rescan-datarace
branch
7 times, most recently
from
f2b6301 to
4ae09a5
Compare
guggero
requested changes
Oct 21, 2022
There was a problem hiding this comment.
Thanks a lot for your work on this! The rescan logic is definitely something that needs to be cleaned up.
But I have to admit, this is very hard to review. All the changes are in a single commit so it's almost impossible to reason about everything at the same time. Even with the new tests added that makes it very hard to make sure no dead lock or new data races are introduced with the changes. And that the formatting doesn't match the rest of the code base doesn't really help the review either.
Can you please split things up a bit more for easier review? Thanks!
Add a NeutrinoChainService interface to provide an abstract implementation of all the methods exported by a *neutrino.ChainService.
Add an internal rescan package for use in the neutrino client to allow abstract access to the public methods of a *neutrino.Rescan.
Add mock implementations for NeutrinoChainService and the rescan Interface.
Add TestNeutrinoClientSequentialStartStop to verify that the neutrino client can have Start() and Stop() called sequentially. Test case fails with -race flag enabled.
Resolve the data race in the existing unit test by removing the variable aliasing on s.rescanErr in notification handler and by only writing ClientConnected notification after the notification handler is running. Write ClientConnected notification via a select statement as a goroutine is unnecessary to send to the unbounded queue.
Add TestNeutrinoClientNotifyReceived to verify that calls to NotifyReceived call the Update method of the rescan object. Verify that there is no race condition on any state variables of the NeutrinoClient. Pass test with -race flag enabled. Add a newRescanner private property of the NeutrinoClient to dynamically inject the rescan constructor. Cannot test without dyanmic dependency injection as the neutrino client previously expected a *neutrino.ChainService struct to create the rescan object. Test with a mockRescan object requires that this assumption be overridden for unit tests.
Move transform of outpoints to a separate method to make Rescan method easier to read.
Add a unit test that demonstrates a segmentation fault exists when concurrent calls to NotifyReceived, NotifyBlocks and Rescan methods of the NeutrinoClient are executed. The rescan property is set to nil and a segmentation fault arises.
Fix segmentation fault by using a channel to share the single instance of the rescan object. TestNeutrinoClientNotifyReceivedRescan passes, but fails with -race flag. Remove rescan and scanning properties and modify test to use rescanCh.
Use a channel of channels to safely share the proper channel to close for quitting the running rescan. Add rescanQuit as a parameter to each of the notification handlers. Wrap the notification handlers as closures and pass the single quit channel to the handlers.
Resolve the data race on s.rescanErr by setting it only once in the constructor and by consuming the error channels returned from starting a rescan process.
Use createRescanner method to create the rescan and rescanQuit channels in a single place.
MStreet3
force-pushed
the
bug/rescan-datarace
branch
from
4eeed4d to
a21bc67
Compare
|
Converting to draft state to address comments from @guggero |
|
See #822 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Background
There is datarace in the neutrino client implementation that
is causing test flakes in
lnd. Specifically, there was a sharedstruct property
rescanwhose updates resulted in a data race withthe possibility of a panic when
rescanwasnil. Further investigationrevealed race conditions on the
rescanQuitandrescanErrprops of the client as well.
Changes
This PR modifies the design of the neutrino client to better reflect
the requirement that only a single
Rescanobject instance berunning at a time. To that end, this PR removes the
rescanpropertyon the client and replaces it with two buffered channels (
cap= 1), one each fora rescanner and the rescanner's signal channel. In this way there is no
need to maintain the state
scanning. Now any method caninfer that the client is scanning if it is able to read a scanner from the channel.
This PR resolves a data race on the
rescanErrchannel via anew error chan consumer method that takes all new error channels
from rescanner restarts and feeds any read errors onto
rescanErr.Testing
This PR adds two interfaces
rescan.InterfaceandNeutrinoChainServiceto abstract out the dependency on structs from the
neutrinopackage.This PR adds a file of mock implementations for these interfaces and uses them
in a new unit test file. The unit tests added are not intended to capture all
the functionality of the client, but to demonstrate that the data races are resolved
and these tests should be run with the
-raceflag enabled.Reviewing
Checkout 93ca685 and run tests.
TestNeutrinoClientNotifyReceivedRescanwill fail with a segmentation faultin the neutrino client due to
rescanbeing set tonil.Checkout 6e1d604 and verify that
the segmentation fault is fixed and
TestNeutrinoClientNotifyReceivedRescanpasses, but that
-raceflag testing fails.Checkout 56af455 run tests and
verify that
TestNeutrinoClientNotifyReceivedRescanpasses with-raceflag enabled.All changes after 56af455 are cosmetic and
cleanup related.
fixes #819