Don't raise an error at close when it's expected

[EliseChouleur]

If session's close raises a SessionHandleInvalid error, that just means the session is already closed by other means.
So don't raise an error but just a warn message in this case.

[hug-dev]

Thanks for the PR!

means the session is already closed by other means

I am wondering if it's possible for the session to be closed by other means if the API has been used only using safe blocks? If not, it could be good to still have this message to show that something unexpected happened?

But maybe I am missing a flow where this could happen!

[wiktor-k]

If not, it could be good to still have this message to show that something unexpected happened?

Yep, it sounds as if there would be a double close in this scenario, which would be good to have in logs.

I wonder about the use case of other means of closing the session... 👀

[hug-dev]

I implemented something similar than what you propose here as part of #326 but checking explicitly is the session is already closed

[EliseChouleur]

Hi !

This happens to me when I remove the physical token before dropping the session. In this case, the session is automatically closed by the library in charge of this token.
To be correct, I think the session isn't really closed but the handle became invalid, that should be why I don't get the SessionClosed error as you're handling in the PR @hug-dev

[hug-dev]

Would my PR suffice then? That would allow you to explicitly call session.close() and filter on the error on your side?

[EliseChouleur]

I wanted to test the behaviour with your PR but the new implementation with the lifetime parameter on the session is a breaking change not compatible with my current implementation with the Pkcs11 context in a mutex and the session in a thread_local ref cell 😅

[hug-dev]

oh no 🙈 Could you maybe produce a minimal example of your usage of Session and Pkcs11 so that we could see how to adapt our code to make it work? Could also add examples to make sure we do not regress

[EliseChouleur]

I've added an example (helped by Claude ^^ so very verbose sorry)
This same example inside your branche gives me those errors:

error[E0106]: missing lifetime specifier
  --> cryptoki/examples/thread_local_session.rs:57:43
   |
57 |     static PKCS11_SESSION: RefCell<Option<Session>> = RefCell::new(None);
   |                                           ^^^^^^^ expected named lifetime parameter
   |
   = help: this function's return type contains a borrowed value, but there is no value for it to be borrowed from
help: consider using the `'static` lifetime, but this is uncommon unless you're returning a borrowed value from a `const` or a `static`, or if you will only have owned values
   |
57 |     static PKCS11_SESSION: RefCell<Option<Session<'static>>> = RefCell::new(None);
   |                                                  +++++++++

error[E0597]: `ctx_lock` does not live long enough
   --> cryptoki/examples/thread_local_session.rs:132:23
    |
131 |             let ctx_lock = PKCS11_CTX.lock().unwrap();
    |                 -------- binding `ctx_lock` declared here
132 |             let ctx = ctx_lock
    |                       ^^^^^^^^ borrowed value does not live long enough
...
159 |         } else {
    |         - `ctx_lock` dropped here while still borrowed
...
169 |     })
    |     - borrow might be used here, when `session_opt` is dropped and runs the destructor for type `RefMut<'_, Option<Session<'_>>>`
    |
    = note: values in a scope are dropped in the opposite order they are defined

Some errors have detailed explanations: E0106, E0597.
For more information about an error, try `rustc --explain E0106`.
error: could not compile `cryptoki` (example "thread_local_session") due to 2 previous errors

[EliseChouleur]

Thanks to the example, a way to resolve the issue I had with your branch is to use a OnceLock for PKCS11 context, I'll try that on my work and get back to you with the outcome :)

[wiktor-k]

I think this looks good 👌 thanks!

[wiktor-k]

I wonder if this would work:

Suggested change

	*PKCS11_CTX.lock().unwrap() = Some(pkcs11);
	*PKCS11_CTX.lock()? = Some(pkcs11);

[wiktor-k]

() should be the default:

Suggested change

	fn generate_and_sign(thread_id: usize) -> testresult::TestResult<()> {
	fn generate_and_sign(thread_id: usize) -> testresult::TestResult {

[wiktor-k]

I think we could import the test result and just reuse it everywhere:

Suggested change

	fn main() -> testresult::TestResult {
	use testresult::TestResult;
	fn main() -> TestResult {

[EliseChouleur]

I've tested my code with your PR @hug-dev, and I can't manage to close the session before the drop:
Logs:

Closing existing thread-local PKCS#11 session
[cryptoki::session::session_management /Users/elisechouleur/dev/rust-cryptoki-hug-dev/cryptoki/src/session/session_management.rs:00023] Failed to close session: Function::CloseSession: PKCS11 error: The specified session handle was invalid at the time that the function was invoked.  Note that this can happen if the session’s token is removed before the function invocation, since removing a token closes all sessions with it.
Error closing PKCS#11 session: Function::CloseSession: PKCS11 error: The specified session handle was invalid at the time that the function was invoked.  Note that this can happen if the session’s token is removed before the function invocation, since removing a token closes all sessions with it.

Code:

            if let Some(session) = session_mut.take() {
                debug!("Closing existing thread-local PKCS#11 session");
                match session.close() {
                    Ok(_) => {
                        debug!("PKCS#11 session closed successfully");
                    }
                    Err(err) => {
                        debug!("Error closing PKCS#11 session: {err}");
                    }
                }
            }