server: use separate goroutines for sotw bidi streams (#530) #531
Conversation
rueian
force-pushed
the
sotw-bidi
branch
3 times, most recently
from
e850faf
to
9815224
Compare
|
@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. |
|
Hi @alecholmez, could you review this if you have free time? |
Yes sorry will review this now |
There was a problem hiding this comment.
So before I continue to review this I'd like to spark a conversation with @snowp, @jpeach, and @sunjayBhatia .
We've been down this path before, I'm not entirely sure we want to add another goroutine since that's just more overhead. We went with the dynamic watch selection to cut down on goroutines actually.
Is there any other way we can approach a fix for the deadlock in linear cache without adding goroutines?
rueian
force-pushed
the
sotw-bidi
branch
3 times, most recently
from
bfbc3b0
to
8e247d6
Compare
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 |
|
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 |
|
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 Please let me know what you think about the test. |
|
taking a look 👍🏽 |
|
Is there any thing I should do to let this PR be merged? |
|
@snowp can you actually give this a look too? I'd like more eyes on this |
|
I was wondering why the delta server won't be deadlocked when I wrote the |
|
ping @snowp |
|
This pull request has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions! |
|
not stale |
|
This pull request has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions! |
|
not stale |
|
This pull request has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions! |
|
Not staled |
|
This pull request has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions! |
|
Not staled |
|
This pull request has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions! |
Signed-off-by: Rueian <rueiancsie@gmail.com>
…roxy#530) Signed-off-by: Rueian <rueiancsie@gmail.com>
Signed-off-by: Rueian <rueiancsie@gmail.com>
To address the #530, I follow the #451 and separate goroutines for both bidi streams.
I also reuse the same
chan cache.Responsefor all watches and remove the reflection.