impl Drop for Session

[jdeinum]

Hi all! Does anyone know if Drop is purposefully not implemented for Session? If not, would people be open to having it implemented? It would be really helpful for RAII patterns and any sort of event loops.

For implementation, we could maybe do something similar to what sqlx does and just spawn the disconnect function as a separate top level task that can be optionally be awaited if needed.

I am happy to open a PR if folks are good with it. Thanks :)

[einarmo]

Doesn't sound entirely unreasonable... The problem is that methods on the Session object aren't actually sending the messages themselves, instead they need to be sent through the EventLoop. If anything, it's the event loop that needs to send disconnects, but that ends up getting a little weird, especially if users just spawn the event loop and forget about it. We may need to be clever about it, perhaps we can check if the session owned by the event loop is the only reference, and send disconnect if that's the case?

In that case we'd also need to handle the case where the event loop is dropped first. Also gets kinda tricky. If you can find a good solution we're happy to look at a PR.

[jdeinum]

Yes that does seem like it'll get tricky.

Do the session and the event loop always share a 1 to 1 relationship? If so, could you wrap both the session event loop and the session inside of an Arc?:

pub async fn connect_to_matching_endpoint(
    &mut self,
    endpoint: impl Into<EndpointDescription>,
    user_identity_token: IdentityToken,
) -> Result<(Arc<Session>, SessionEventLoop), StatusCode> { 
    // ...
}

into:

pub struct SessionWithEventLoop {
    pub session: Session,
    pub event_loop: SessionEventLoop,
}

pub async fn connect_to_matching_endpoint(
    &mut self,
    endpoint: impl Into<EndpointDescription>,
    user_identity_token: IdentityToken,
) -> Result<(Arc<SessionWithEventLoop> StatusCode> {
    // ...
}

Then we could implement Drop for the SessionWithEventLoop:

impl Drop for SessionWithEventLoop {
    fn drop(&mut self) {
        // still have both a session and event loop here 
        // send whatever message we need 
        // before both of the inner values are dropped
    }
}

I think this could work if they are 1 to 1. But you would (maybe) need to prevent getting a reference to the inner types. If SessionWithEventLoop derefs into session, then you can still access everything you would need normally. Also I think you'd have to spawn the event loop within the constructor to prevent multiple references from trying to run the event loop.

What are your thoughts on something like this? I assume it will be a pretty big change for questionable gain.

[einarmo]

It's pretty fundamental to the design of the client that you have a unique reference to the event loop, and a shared reference to the session. I'm not sure that's something we can change. In fact, at the moment, the event loop isn't even available to the user while the client is running. SessionEventLoop::enter consumes the event loop object and returns a stream.

I do think we'd need a more complex solution:

1. From the connect methods, return a wrapper around Session containing a lease that when dropped, signals some event that we can listen to in the SessionEventLoop. This would have to deref to Session. I dislike how breaking this is, as it modifies the type every client uses for everything, but we can't just implement Drop for Session directly, since we hold references to the session from multiple places in the SessionEventLoop.
2. If we're signaled while the event loop is still running, we can assume that the event loop is still being polled, and just terminate normally.
3. In the internal type event_loop::ConnectedState: Track whether the connection was closed for whatever reason, and if it wasn't closed while the ConnectedState was dropped, spawn a task to close the connection. This may need to be opt-out, just to be on the safe side.

It's not trivial, in other words, but I do think something like this could work. ConnectedState holds enough of the connection to terminate the session on its own.

[jdeinum]

Thanks, that helps clear things up. I read through the SessionEventLoop code and think I understand what you're getting at.

So either the wrapper would need to hold a watch::Sender<T> and send a value on Drop, or the wrapper could hold a reference back to the client similar to similar to Ref and on drop we can just directly change the state of the event loop?

If that sounds about right, I can take a look at trying to implement it over the next 3 days or so, and see what I can come up with.

[jdeinum]

I ended up just calling disconnect inside error conditions (i.e if let Err(e) = ..., which may not be as nice but does the job for now.

I'm still interested in implementing this, but I don't know what my timeline looks like. If you'd like, you can close this for the time being and if I come up with something reasonable then we can reopen it?

[einarmo]

No need to close the issue, it makes sense as a potential improvement.