Fix/5928 header dependency

[ak88]

Fixes Closes Resolves #

Because HeadersSyncFeed is multithreaded, when running HandleResponse, there is a race condition where two threads can attempt to add the same batch to the _dependencies collection. HandleResponse will guard itself by removing the batch from _sent collection before calling InsertHeaders, but at the same time PrepareRequest can call InsertHeaders.

Changes

Upgrade PrepareRequest read lock to a write lock.

What types of changes does your code introduce?

• [x] Bugfix (a non-breaking change that fixes an issue)
• [ ] New feature (a non-breaking change that adds functionality)
• [ ] Breaking change (a change that causes existing functionality not to work as expected)
• [ ] Optimization
• [ ] Refactoring
• [ ] Documentation update
• [ ] Build-related changes
• [ ] Other: Description

Testing

Requires testing

• [ ] Yes
• [ ] No

If yes, did you write tests?

• [ ] Yes
• [ ] No

Notes on testing

This can affect perf on sync, and @kamilchodola is currently running tests.

Documentation

Requires documentation update

• [ ] Yes
• [x] No

Requires explanation in Release Notes

• [ ] Yes
• [x] No

[asdacap]

Good catch. Got another idea? The writelock is suppose to be use only during reset.

[ak88]

Good catch. Got another idea? The writelock is suppose to be use only during reset.

Hm could introduce a new lock around HandleResponse and PrepareRequest. But I think the right, but time consuming way, would be to refactor into something like a more dedicated Resequencer pattern using System.Threading.Channels .

[asdacap]

InsertHeaders in HandleResponse is wrapped in a lock (_handlerLock) but InsertHeaders in HandleDependentBatches does not. Maybe thats the problem?

[ak88]

InsertHeaders in HandleResponse is wrapped in a lock (_handlerLock) but InsertHeaders in HandleDependentBatches does not. Maybe thats the problem?

I dont think that is sufficient to solve the problem, since the lock is taking after the ´_sent´ check in HandleResponse . So one thread could be waiting for the lock while PrepareRequest adds the batch again to _sent , which makes another HandleResponse able to enter.

[LukaszRozmej]

_dependencies is ConcurrentDictionary, so can we just use TryAdd or AddOrUpdate and handle duplicates?

[ak88]

_dependencies is ConcurrentDictionary, so can we just use TryAdd or AddOrUpdate and handle duplicates?

Yes, that is another solution. However it feels a little "hackish", since the original author specifically checks for that condition, and throws InvalidOperationException .

I dont mind going with this though.

[ak88]

@asdacap @LukaszRozmej Changed to ignore the batch if its already in _dependencies

I will ask Kamil to run some tests.

[benaadams]

Is this still interesting?

[ak88]

Is this still interesting?

Its been refactored since i made this, but the race condition is still there so i merged master. @asdacap could you please take a look ?