Skip to content

Conversation

lukebakken
Copy link
Collaborator

Part of #1682

@lukebakken lukebakken self-assigned this Oct 18, 2024
@lukebakken lukebakken added this to the 7.0.0 milestone Oct 18, 2024
@danielmarbach
Copy link
Collaborator

I have looked at the client code changes that have been recently added in addition to those changes and overall the inlining of the channel base and the partial split helps to better navigate the complexity of the code.

My biggest concern after reading through the changes is that all the flag checking and the hidden acquiring and releasing of the confirmation semaphore plus the manipulation of the sequence number in methods that assume the semaphore is held makes it non-trivial to understand the flow. I think this structure could quickly become a maintenance burden overtime and be the source of subtle bugs.

I could not spend more time digging into the code because the time I had available was eaten up by providing the spike. Once those discussions settle, I might find some time to think how the code can be restructured.

@lukebakken
Copy link
Collaborator Author

My biggest concern after reading through the changes is that all the flag checking and the hidden acquiring and releasing of the confirmation semaphore plus the manipulation of the sequence number in methods that assume the semaphore is held makes it non-trivial to understand the flow.

I agree. I will add a lot more comments to the code. At this point, unless a bug or improvement is found, I'd like to keep it as is because it's all "behind the scenes" anyway and we can change it in the future.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants