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: Ensure pending messages are delivered when OutputPort is dropped. #293

Merged
merged 2 commits into from
Nov 27, 2024

Conversation

stuartcarnie
Copy link
Contributor

Summary

This PR ensures that pending messages are delivered when an OutputPort is dropped.

Overview

This PR removes the Drop implementation from OutputPort<T>, which called the stop function on all subscribers:

fn drop(&mut self) {
let mut subs = self.subscriptions.write().unwrap();
for sub in subs.iter_mut() {
sub.stop();
}
subs.clear();
}

Calling the stop function would abort the JoinHandle of the subscriber:

pub(crate) fn stop(&mut self) {
self.handle.abort();
}

which would immediately terminate the tokio task responsible for delivering messages to the subscriber:

let handle = crate::concurrency::spawn(async move {
while let Ok(Some(msg)) = port.recv().await {
if let Some(new_msg) = converter(msg) {
if receiver.cast(new_msg).is_err() {
// kill the subscription process, as the forwarding agent is stopped
return;
}
}
}
});

Removing the Drop implementation still results in the tasks properly shutting down, as per the tokio documentation for closing broadcast channels:

Closing

When all Sender handles have been dropped, no new values may be sent. At this point, the channel is “closed”. Once a receiver has received all values retained by the channel, the next call to recv will return with RecvError::Closed.

The implementation of the receive pump will exit when the channel does not return an Ok result:

while let Ok(Some(msg)) = port.recv().await {

Comment on lines +161 to +163
#[crate::concurrency::test]
#[tracing_test::traced_test]
async fn test_delivery() {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test consistently fails with the version on the main branch, and passes when Drop is removed

Copy link
Owner

@slawlor slawlor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! Thanks for the contribution

Copy link

codecov bot commented Nov 27, 2024

Codecov Report

Attention: Patch coverage is 93.18182% with 3 lines in your changes missing coverage. Please review.

Project coverage is 81.33%. Comparing base (1949d92) to head (811f629).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
ractor/src/port/output/tests.rs 93.18% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #293      +/-   ##
==========================================
+ Coverage   81.30%   81.33%   +0.03%     
==========================================
  Files          60       60              
  Lines       10856    10890      +34     
==========================================
+ Hits         8826     8857      +31     
- Misses       2030     2033       +3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@slawlor
Copy link
Owner

slawlor commented Nov 27, 2024

Please run rustfmt then update the pr, then it's good to merge!

@stuartcarnie
Copy link
Contributor Author

@slawlor sorry I missed that – done!

@slawlor slawlor merged commit cd7de35 into slawlor:main Nov 27, 2024
11 checks passed
@slawlor
Copy link
Owner

slawlor commented Nov 27, 2024

I'll try and get a version cut today with this fix included

@philjb
Copy link

philjb commented Nov 27, 2024

I'll try and get a version cut today with this fix included

appreciate it @slawlor!

@slawlor
Copy link
Owner

slawlor commented Nov 27, 2024

V0.13.2 has this included.

@stuartcarnie stuartcarnie deleted the fix_output_port branch November 27, 2024 19:59
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.

3 participants