storagenode/orders: fix data race in settle

[egonelbre]

errList was being modified from two goroutines, use a separate errs.Group to capture errors. Unfortunately, errs.Group cannot be returned easily from group.Go, which would allow to make this bug less likely.

Please describe the tests:

• Test 1:
• Test 2:

Please describe the performance impact:

Code Review Checklist (to be filled out by reviewer)

• Does the PR describe what changes are being made?
• Does the PR describe why the changes are being made?
• Does the code follow our style guide?
• Does the code follow our testing guide?
• Is the PR appropriately sized? (If it could be broken into smaller PRs it should be)
• Does the new code have enough tests? (every PR should have tests or justification otherwise. Bug-fix PRs especially)
• Does the new code have enough documentation that answers "how do I use it?" and "what does it do?"? (both source documentation and higher level, diagrams?)
• Does any documentation need updating?
• Do the database access patterns make sense?

[ifraixedes]

Couldn't we use a buffered of error elements channel of len(orders) capacity and iterate over it after group.Wait() excluding nil element and accumulate them errList how it's done now?

[egonelbre]

Why? This sounds like a much more complicated approach than it's currently using.

[ifraixedes]

isn't sendErrors.Add called concurrently? of if it's .Add is safe to be called concurrently?

[egonelbre]

sendErrors.Add is not called concurrently

[ifraixedes]

oh yeah, I saw the loop and the group in inverse order.
I've got confused with the "unfortunately" part of the initial message.

[VinozzZ]

I'm confused now. why is sendErrors.Add not called concurrently?

[egonelbre]

There is only goroutine using sendErrors at a time at all times. I.e. it's called from another goroutine, but there's only one of those.... so technically no concurrent calls.

[ifraixedes]

Because it's only called inside of group.Go and it's read after the group ends.

I was mistaken because my brain lied to me making me see the for loop before group.Go so I thought that several goroutines created by several calls to group.Go were running, but the loop it's inside so there is only one goroutine.

[ifraixedes]

oh yeah, I saw the loop and the group in inverse order.
I've got confused with the "unfortunately" part of the initial message.

[VinozzZ]

LGTM