Sessions aren't automatically closed on Drop

[ipaljak-tbtl]

Hi,

I've noticed that, contrary to documentation, yubihsm::session::Session doesn't get closed when dropped.

For example:

    #[test]
    fn sessions_are_not_released_on_drop() {
        for i in 0..20 {
            let http_config = yubihsm::connector::HttpConfig::default();
            let connector = yubihsm::Connector::http(&http_config);
            let credentials = yubihsm::Credentials::from_password(1, b"password");
            let client = yubihsm::client::Client::open(connector, credentials, false).unwrap();
            println!("Constructed {:?}-th client", i);
            drop(client);
        }
    }

results in:

running 1 test
Constructed 0-th client
Constructed 1-th client
Constructed 2-th client
Constructed 3-th client
Constructed 4-th client
Constructed 5-th client
Constructed 6-th client
Constructed 7-th client
Constructed 8-th client
Constructed 9-th client
Constructed 10-th client
Constructed 11-th client
Constructed 12-th client
Constructed 13-th client
Constructed 14-th client
Constructed 15-th client
thread 'tests::sessions_are_not_released_on_drop' panicked at 'called `Result::unwrap()` on an `Err` value: Error(Context { kind: DeviceError, source: Some(Error(Context { kind: DeviceError, source: Some(SessionsFull) })) })', src/main.rs:17:87

Ive also confirmed that implementing a custom Drop on Session where self.close() is explicitly called fixes this issue.

Would you mind accepting a PR with this fix?

[tony-iqlusion]

See #265 for some history

[ipaljak-tbtl]

Thanks for the response.

I've tried to follow through the discussions in the issues/PR you've linked but I'm still not entirely sure what's the state with the TKMMS issue (iqlusioninc/tmkms#37), and I'm probably missing some context beecause I'm not using TKMMS :)

Anyway, from what I understand, #273 turned out to be the bugfix for the issue? Does that mean its safe to simply revert #265?

If not, what is the intended way to close the session with the current API?

[tony-iqlusion]

Sorry, I should've given more background.

While it's possible to revert #265 since the actual culprit was fixed in #273, it's lead me to question the wisdom of executing complicated code in drop handlers.

A panic in a drop handler is extremely painful to debug, which is why the original code was using panic::catch_unwind and attempting to at least log the panic, but that could potentially panic as well.

I guess we could try reverting it and see if any problems arise. Things seem nicely stable now though which is why I worry a bit about it.