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

[Bug]: EventListener will lose event since it's not cancellation safe. #339

Closed
1 task done
Phoenix500526 opened this issue Jun 21, 2023 · 0 comments · Fixed by #340
Closed
1 task done

[Bug]: EventListener will lose event since it's not cancellation safe. #339

Phoenix500526 opened this issue Jun 21, 2023 · 0 comments · Fixed by #340
Assignees
Labels
bug Something isn't working
Milestone

Comments

@Phoenix500526
Copy link
Collaborator

Description about the bug

When using select! in a loop to receive messages from multiple sources, we should make sure that the receive call is cancellation safe to avoid losing messages. Unfortunately, EventListener is not cancellation safe. That means some implementation in our code may lose event due to the select! cancel it.

Here is the task method in watch_server.rs, and its implementation lead to events loss.

    async fn task<ST, W>(
        next_id_gen: Arc<WatchIdGenerator>,
        kv_watcher: Arc<W>,
        res_tx: mpsc::Sender<Result<WatchResponse, tonic::Status>>,
        mut req_rx: ST,
        header_gen: Arc<HeaderGenerator>,
        watch_progress_notify_interval: Duration,
    ) where
        ST: Stream<Item = Result<WatchRequest, tonic::Status>> + Unpin,
        W: KvWatcherOps,
    {
        let (event_tx, mut event_rx) = mpsc::channel(CHANNEL_SIZE);
        let stop_notify = Arc::new(Event::new());
        let mut watch_handle = WatchHandle::new(
            kv_watcher,
            res_tx,
            event_tx,
            Arc::clone(&stop_notify),
            next_id_gen,
            header_gen,
        );
        let mut ticker = tokio::time::interval(watch_progress_notify_interval);
        loop {
            tokio::select! {
                req = req_rx.next() => {
                    if let Some(req) = req {
                        match req {
                            Ok(req) => {
                                watch_handle.handle_watch_request(req).await;
                            }
                            Err(e) => {
                                warn!("Receive WatchRequest error {:?}", e);
                                break;
                            }
                        }
                    } else {
                        warn!("Watch client closes connection");
                        break;
                    }
                }
                event = event_rx.recv() => {
                    if let Some(event) = event {
                        watch_handle.handle_watch_event(event).await;
                    } else {
                        panic!("Watch event sender is closed");
                    }
                }
                _ = ticker.tick() => {
                    watch_handle.handle_tick_progress().await;
                }
                _ = stop_notify.listen() => {
                    break;
                }
            }
        }
    }
}

FYI: https://docs.rs/tokio/1.28.2/tokio/macro.select.html

Version

0.4.1 (Default)

Relevant log output

No response

Code of Conduct

  • I agree to follow this project's Code of Conduct
@Phoenix500526 Phoenix500526 added the bug Something isn't working label Jun 21, 2023
@Phoenix500526 Phoenix500526 self-assigned this Jun 21, 2023
Phoenix500526 added a commit to Phoenix500526/Xline that referenced this issue Jun 21, 2023
Phoenix500526 added a commit to Phoenix500526/Xline that referenced this issue Jun 21, 2023
@Phoenix500526 Phoenix500526 linked a pull request Jun 21, 2023 that will close this issue
Phoenix500526 added a commit to Phoenix500526/Xline that referenced this issue Jun 22, 2023
@Phoenix500526 Phoenix500526 added this to the v0.5.0 milestone Jun 22, 2023
Phoenix500526 added a commit to Phoenix500526/Xline that referenced this issue Jun 24, 2023
Phoenix500526 added a commit to Phoenix500526/Xline that referenced this issue Jun 25, 2023
@mergify mergify bot closed this as completed in #340 Jun 26, 2023
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 a pull request may close this issue.

1 participant