Skip to content

serialise disconnect logic #886

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Feb 26, 2024
Merged

serialise disconnect logic #886

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1ac2c8d
serialise disconnect logic
Shubham8287 Feb 23, 2024
cfd8942
Keep the ModuleHost::disconnect_client method
coolreader18 Feb 26, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion crates/client-api/src/routes/subscribe.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -306,7 +306,12 @@ async fn ws_client_actor(client: ClientConnection, mut ws: WebSocketStream, mut
// https://rust-lang.github.io/wg-async/vision/submitted_stories/status_quo/aws_engineer/solving_a_deadlock.html
handle_queue.clear();

client.module.disconnect_client(client.id).await;
// ignore NoSuchModule; if the module's already closed, that's fine
let _ = client.module.subscriptions().remove_subscriber(client.id);
let _ = client
.module
.call_identity_connected_disconnected(client.id.identity, client.id.address, false)
.await;
}

enum ClientMessage {
Expand Down
11 changes: 1 addition & 10 deletions crates/core/src/host/module_host.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ use tokio::sync::oneshot;

use super::host_controller::HostThreadpool;
use super::{ArgsTuple, InvalidReducerArguments, ReducerArgs, ReducerCallResult, ReducerId, Timestamp};
use crate::client::{ClientActorId, ClientConnectionSender};
use crate::client::ClientConnectionSender;
use crate::database_logger::LogLevel;
use crate::db::datastore::traits::{TxData, TxOp};
use crate::db::relational_db::RelationalDB;
Expand Down Expand Up @@ -500,15 +500,6 @@ impl ModuleHost {
Ok(rx.await.expect("instance panicked"))
}

pub async fn disconnect_client(&self, client_id: ClientActorId) {
tokio::join!(
Copy link
Collaborator

@coolreader18 coolreader18 Feb 26, 2024

Choose a reason for hiding this comment

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

Oh, it looks like they were never really concurrent anyway. I don't know why I thought they were; remove_subscriber is not an async function.

Copy link
Contributor Author

@Shubham8287 Shubham8287 Feb 26, 2024

Choose a reason for hiding this comment

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

lol, anyway this doesn't solve the deadlock, but anyway we can merge it?

async { self.subscriptions().remove_subscriber(client_id) },
self.call_identity_connected_disconnected(client_id.identity, client_id.address, false)
// ignore NoSuchModule; if the module's already closed, that's fine
.map(drop)
);
}

pub async fn call_identity_connected_disconnected(
&self,
caller_identity: Identity,
Expand Down