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

refactor: do not cancel the task returned from async_imap Handle.wait_with_timeout #6526

Merged
merged 1 commit into from
Feb 10, 2025

Conversation

link2xt
Copy link
Collaborator

@link2xt link2xt commented Feb 8, 2025

This task is not guaranteed to be cancellation-safe and provides a stop token for safe cancellation instead. We should always cancel the task properly
and not by racing against another future.
Otherwise following call to Handle.done()
may work on IMAP session that is in the middle
of response, for example.

@link2xt
Copy link
Collaborator Author

link2xt commented Feb 8, 2025

This fixes some code that did not look right: #6477 (comment)

It should not result into infinite loop in any case, however.

@link2xt link2xt force-pushed the link2xt/idle-cancel branch from a7b69cd to 605525f Compare February 8, 2025 23:49
Copy link
Collaborator

@Hocuri Hocuri left a comment

Choose a reason for hiding this comment

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

From reading the source, wait_with_timeout() does pretty much look cancellation-safe to me. The returned future only uses await in two places:

                let Ok(res) = timeout(dur, interruptible_stream.next()).await else {

this is using timeout() itself, so it's obviously cancellation-safe.

                        handle_unilateral(resp, sender.clone()).await;

handle_unilateral() is async in order to send() unsolicited responses into the channel. Not sure if that means that cancelling can cause unsolicited responses to get lost, but since we only use unsolicited responses to figure out whether we should fetch again, and we always fetch again after idle, this should be fine for us.

So, I'm like +/-0 on this PR since on the one hand it makes us use the API in a more "correct" way, OTOH it changes things without any pressing reason, which can always make things worse. I'll approve it anyway so that you can decide to merge it.

let context = context.clone();
let folder = folder.to_string();

tokio::spawn(async move {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just a note that we wouldn't need tokio::spawn() here, tokio::join!()ing the two futures would suffice. But the current code has the advantage that it's only using simple concepts, whereas tokio::join!() is somewhat of a harder to grasp concept, so, I'm not actually proposing to change anything.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm not sure how I can cancel the second "relay" future once I have put it into join already.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Something with join set is probably possible, but the task is good enough.

src/imap/idle.rs Outdated Show resolved Hide resolved
src/imap/idle.rs Outdated Show resolved Hide resolved
@link2xt
Copy link
Collaborator Author

link2xt commented Feb 10, 2025

this is using timeout() itself, so it's obviously cancellation-safe.

I am not so sure everything async-imap does internally is correct as well, it needs review too. Wrapping reading of IMAP connection into timeout may be incorrect.

A safer way to terminate IDLE would be to send DONE on timeout, set read timeout on a socket and wait until either read times out (because connection is lost) or until IDLE terminates in a clean way.

@link2xt link2xt force-pushed the link2xt/idle-cancel branch from 605525f to fb9a9ae Compare February 10, 2025 20:02
@link2xt
Copy link
Collaborator Author

link2xt commented Feb 10, 2025

I am pretty sure that this does not fix any bug, but I want to get rid of all future cancellations on code paths that keep Session (or better all code paths) because reasoning about future cancellations is very difficult and we have the bug with something going into infinite loop around this area.

@link2xt link2xt changed the title fix: do not cancel the task returned from async_imap Handle.wait_with_timeout refactor: do not cancel the task returned from async_imap Handle.wait_with_timeout Feb 10, 2025
@link2xt link2xt force-pushed the link2xt/idle-cancel branch from fb9a9ae to 5e22af2 Compare February 10, 2025 20:07
…t_with_timeout`

This task is not guaranteed to be cancellation-safe
and provides a stop token for safe cancellation instead.
We should always cancel the task properly
and not by racing against another future.
Otherwise following call to Handle.done()
may work on IMAP session that is in the middle
of response, for example.
@link2xt link2xt force-pushed the link2xt/idle-cancel branch from 5e22af2 to 81e9628 Compare February 10, 2025 20:12
@link2xt link2xt merged commit 81e9628 into main Feb 10, 2025
37 checks passed
@link2xt link2xt deleted the link2xt/idle-cancel branch February 10, 2025 20:42
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