Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix Circular Reference issues #65

Merged
merged 1 commit into from
Jun 26, 2024

Conversation

Xylaant
Copy link
Contributor

@Xylaant Xylaant commented Jun 20, 2024

Fixed circular reference issues preventing DuplexSockets from being cleaned up correctly.

Motivation:

I found two separate circular references while working on an application.

  1. The DuplexSocket created a task which was cleaning up abort handles for RequestResposne Responders. The problem was this task kept a clone of the duplex socket, including the Tx channel that the channel used. Since the Tx end of the channel was being held by the task that also held the RX end of the channel, this task would never shut down, and none of the other contents of the DuplexSocket would be dropped either

  2. When a user saved the RSocket provided to the on_setup acceptor, this caused an issue when the remote disconnected. The read tasks for the socket would complete, but the write tasks would keep the socket open for a time. The write tasks couldn't expire because a reference to DuplexSocket was kept within the RSocket provided to the user.

Modifications:

  1. To solve this, I removed the task altogether, and rewrote the request resposne handler to behave like the request channel and request stream responders - It put an abort handle in the map of handles, just like the others. This allowed me to remove the problematic task
  2. To solve this, I restructured DuplexSocket a bit - There is now a private DuplexSocketInner, and DuplexSocket contains an Arc to that struct. I also created a ClientRequester and ServerRequester, which implement RSocket. The intent is that users (clients or servers) will be given one of the requesters. The ServerRequester, in particular, only holds a weak reference to the DuplexSocketInner, so the DuplexSocketInner can be dropped once the remote socket is closed.

Finally, there were a few places where I turned async functions into non-async functions.

Result:

This should prevent issues where some of the underling tasks remain running indefinitely on disconnection, and prevent possible reference cycles from being created inadvertently.

Provide user with a weak reference to Requester, when in Server mode, to allow socket to close properly
@Xylaant Xylaant changed the title Remove cancel task, which was causing a circular reference Fix Circular Reference issues Jun 20, 2024
@jjeffcaii jjeffcaii added the bug Something isn't working label Jun 25, 2024
@jjeffcaii jjeffcaii self-assigned this Jun 25, 2024
@jjeffcaii
Copy link
Member

Thanks for the PR 👍🏽

@jjeffcaii jjeffcaii merged commit ebbafca into rsocket:master Jun 26, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants