rsocket / rsocket-rust

RSocket Rust Implementation using Tokio
Apache License 2.0
209 stars 19 forks source link

Fix Circular Reference issues #65

Closed Xylaant closed 3 months ago

Xylaant commented 3 months ago

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.

jjeffcaii commented 3 months ago

Thanks for the PR 👍🏽