go-control-plane server: use separate goroutines for sotw bidi streams (#530)

To address the #530, I follow the #451 and separate goroutines for both bidi streams.

I also reuse the same chan cache.Response for all watches and remove the reflection.

Dec 21 '21 09:12 rueian

@rueian can you rebase your branch of what was merged? The PR this was based off of is now in main

Jan 06 '22 20:01 alecholmez

@rueian can you rebase your branch of what was merged? The PR this was based off of is now in main

Sure, I have rebased it. Thanks.

Jan 07 '22 03:01 rueian

Hi @alecholmez, could you review this if you have free time?

Jan 10 '22 10:01 rueian

Hi @alecholmez, could you review this if you have free time?

Yes sorry will review this now

Jan 11 '22 20:01 alecholmez

Is there any other way we can approach a fix for the deadlock in linear cache without adding goroutines?

currently every resource update causes a response to be sent via sending on the responder channels, we could batch and coalesce updates with a timer to ensure multiple updates wont be blocked etc. before a response is actually sent

Jan 12 '22 21:01 sunjayBhatia

also would be nice to have a test that demonstrates this issue, with the new refactor as well, having a harder time piecing together the sequence of events, honestly havent used/looked at the linearcache much myself

Jan 12 '22 22:01 sunjayBhatia

Hi @sunjayBhatia,

Demonstrating the issue is not easy and it is kind of flaky. However, I have still tried to make a test case to do that.

The TestSOTWLinearCacheIntegrationDeadLock test case just tries to simulate what could happen to the LinearCache and SOTW server concurrently with many iterations. And I never pass the test with previous version of SOTW server.

Please let me know what you think about the test.

Jan 13 '22 16:01 rueian

taking a look 👍🏽

Jan 13 '22 20:01 sunjayBhatia

Hi @alecholmez, @sunjayBhatia

Is there any thing I should do to let this PR be merged?

Jan 20 '22 16:01 rueian

@snowp can you actually give this a look too? I'd like more eyes on this

Feb 05 '22 05:02 alecholmez

I was wondering why the delta server won't be deadlocked when I wrote the TestSOTWLinearCacheIntegrationDeadLock test case. And it turns out that the delta server also uses separated goroutine: https://github.com/envoyproxy/go-control-plane/blob/main/pkg/server/delta/v3/server.go#L176-L181

Feb 14 '22 07:02 rueian

ping @snowp

Mar 04 '22 15:03 alecholmez