From 54f00dc05a7033ebc257d592ed9674462539d208 Mon Sep 17 00:00:00 2001 From: Ivan Enderlin Date: Wed, 25 Jan 2023 09:56:21 +0100 Subject: [PATCH 1/3] feat(crypto-nodejs) Implement `OlmMachine.close`. This patch fixes https://github.com/matrix-org/matrix-rust-sdk/issues/1379/. This is similar to https://github.com/matrix-org/matrix-rust-sdk/pull/1197/. We need to close the `OlmMachine`. Problem, `napi-rs` doesn't allow methods to take ownership of `self`, so the following code: ```rs fn close(self) {} ```` isn't allowed. There an [`ObjectFinalize`](https://docs.rs/napi/latest/napi/bindgen_prelude/trait.ObjectFinalize.html) trait, but it's only called when the JS value is being collected by the GC. It's not possible to force call `finalize` here. This patch then introduces a new type: ```rs enum OlmMachineInner { Opened(matrix_sdk_crypto::OlmMachine), Closed } ``` that derefs to `matrix_sdk_crypto::OlmMachine` when its variant is `Opened`, otherwise it panics. Calling the new `OlmMachine::close` method will change the inner state from `Opened` to `Closed`. Ideally, I wanted to throw an error instead of panicking, but `napi_env` (used to build an `Env`, used to throw an error) is neither `Sync` nor `Send`. Honestly, my knowledge of NodeJS and NAPI is too weak to implement `Sync` and `Send` for `*mut napi_env__` safely. Especially because it can be executed from various threads within `async fn` functions, that are driven by Tokio in `napi-rs`. So, yeah, for the moment, it panics! --- .../matrix-sdk-crypto-nodejs/src/machine.rs | 46 +++++++++++++++++-- .../tests/machine.test.js | 14 +++++- 2 files changed, 55 insertions(+), 5 deletions(-) diff --git a/bindings/matrix-sdk-crypto-nodejs/src/machine.rs b/bindings/matrix-sdk-crypto-nodejs/src/machine.rs index ec01b769d..d531ce1ab 100644 --- a/bindings/matrix-sdk-crypto-nodejs/src/machine.rs +++ b/bindings/matrix-sdk-crypto-nodejs/src/machine.rs @@ -2,6 +2,7 @@ use std::{ collections::{BTreeMap, HashMap}, + ops::Deref, sync::Arc, }; @@ -16,11 +17,38 @@ use crate::{ sync_events, types, vodozemac, }; +/// The value used by the `OlmMachine` JS class. +/// +/// It has 2 states: `Opened` and `Closed`. Why maintaining the state here? +/// Because NodeJS has no way to drop an object explicitely, and we want to be +/// able to “close” the `OlmMachine` to free all associated data. More over, +/// `napi-rs` doesn't allow a function to take the ownership of the type itself +/// (`fn close(self) { … }`). So we manage the state ourselves. +/// +/// Using the `OlmMachine` when its state is `Closed` will panic. +enum OlmMachineInner { + Opened(matrix_sdk_crypto::OlmMachine), + Closed, +} + +impl Deref for OlmMachineInner { + type Target = matrix_sdk_crypto::OlmMachine; + + #[inline] + fn deref(&self) -> &Self::Target { + match self { + Self::Opened(machine) => machine, + Self::Closed => panic!("The `OlmMachine` has been closed, cannot use it anymore"), + } + } +} + /// State machine implementation of the Olm/Megolm encryption protocol /// used for Matrix end to end encryption. +// #[napi(custom_finalize)] #[napi] pub struct OlmMachine { - inner: matrix_sdk_crypto::OlmMachine, + inner: OlmMachineInner, } #[napi] @@ -77,7 +105,7 @@ impl OlmMachine { store_passphrase.zeroize(); Ok(OlmMachine { - inner: match store { + inner: OlmMachineInner::Opened(match store { Some(store) => matrix_sdk_crypto::OlmMachine::with_store( user_id.inner.as_ref(), device_id.inner.as_ref(), @@ -92,7 +120,7 @@ impl OlmMachine { ) .await } - }, + }), }) } @@ -407,4 +435,16 @@ impl OlmMachine { pub async fn sign(&self, message: String) -> types::Signatures { self.inner.sign(message.as_str()).await.into() } + + /// Shut down the `OlmMachine`. + /// + /// The `OlmMachine` cannot be used after this method has been called, + /// otherwise it will panic. + /// + /// All associated resources will be closed too, like the crypto storage + /// connections. + #[napi(strict)] + pub fn close(&mut self) { + self.inner = OlmMachineInner::Closed; + } } diff --git a/bindings/matrix-sdk-crypto-nodejs/tests/machine.test.js b/bindings/matrix-sdk-crypto-nodejs/tests/machine.test.js index a3b424683..92dc5f8a4 100644 --- a/bindings/matrix-sdk-crypto-nodejs/tests/machine.test.js +++ b/bindings/matrix-sdk-crypto-nodejs/tests/machine.test.js @@ -25,7 +25,7 @@ describe(OlmMachine.name, () => { expect(await OlmMachine.initialize(new UserId('@foo:bar.org'), new DeviceId('baz'), temp_directory, 'hello')).toBeInstanceOf(OlmMachine); }); }); - + const user = new UserId('@alice:example.org'); const device = new DeviceId('foobar'); const room = new RoomId('!baz:matrix.org'); @@ -34,6 +34,16 @@ describe(OlmMachine.name, () => { return OlmMachine.initialize(new_user || user, new_device || device); } + test('can drop/close, and then re-open', async () => { + const temp_directory = await fs.mkdtemp(path.join(os.tmpdir(), 'matrix-sdk-crypto--')); + + let m1 = await OlmMachine.initialize(new UserId('@test:bar.org'), new DeviceId('device'), temp_directory, 'hello') + m1.close(); + + let m2 = await OlmMachine.initialize(new UserId('@test:bar.org'), new DeviceId('device'), temp_directory, 'hello') + m2.close(); + }); + test('can read user ID', async () => { expect((await machine()).userId.toString()).toStrictEqual(user.toString()); }); @@ -179,7 +189,7 @@ describe(OlmMachine.name, () => { beforeAll(async () => { m = await machine(user, device); }); - + test('can pass keysquery and keysclaim requests directly', async () => { { // derived from https://github.com/matrix-org/matrix-rust-sdk/blob/7f49618d350fab66b7e1dc4eaf64ec25ceafd658/benchmarks/benches/crypto_bench/keys_query.json From bac105a9aa31610f095a746ebd7c19fe48a93950 Mon Sep 17 00:00:00 2001 From: Ivan Enderlin Date: Thu, 26 Jan 2023 09:59:15 +0100 Subject: [PATCH 2/3] doc(crypto-nodejs): Fix a typo. --- bindings/matrix-sdk-crypto-nodejs/src/machine.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/bindings/matrix-sdk-crypto-nodejs/src/machine.rs b/bindings/matrix-sdk-crypto-nodejs/src/machine.rs index d531ce1ab..288e57e64 100644 --- a/bindings/matrix-sdk-crypto-nodejs/src/machine.rs +++ b/bindings/matrix-sdk-crypto-nodejs/src/machine.rs @@ -20,7 +20,7 @@ use crate::{ /// The value used by the `OlmMachine` JS class. /// /// It has 2 states: `Opened` and `Closed`. Why maintaining the state here? -/// Because NodeJS has no way to drop an object explicitely, and we want to be +/// Because NodeJS has no way to drop an object explicitly, and we want to be /// able to “close” the `OlmMachine` to free all associated data. More over, /// `napi-rs` doesn't allow a function to take the ownership of the type itself /// (`fn close(self) { … }`). So we manage the state ourselves. From 6593f615569d7f29a418f83cb7ff4419651f1394 Mon Sep 17 00:00:00 2001 From: Ivan Enderlin Date: Wed, 15 Feb 2023 09:15:31 +0100 Subject: [PATCH 3/3] doc(crypto-nodejs): Add safety comment for `OlmMachine.close`. --- bindings/matrix-sdk-crypto-nodejs/src/machine.rs | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/bindings/matrix-sdk-crypto-nodejs/src/machine.rs b/bindings/matrix-sdk-crypto-nodejs/src/machine.rs index 288e57e64..daf3bec4b 100644 --- a/bindings/matrix-sdk-crypto-nodejs/src/machine.rs +++ b/bindings/matrix-sdk-crypto-nodejs/src/machine.rs @@ -443,6 +443,11 @@ impl OlmMachine { /// /// All associated resources will be closed too, like the crypto storage /// connections. + /// + /// # Safety + /// + /// The caller is responsible to **not** use any objects that came from this + /// `OlmMachine` after this `close` method has been called. #[napi(strict)] pub fn close(&mut self) { self.inner = OlmMachineInner::Closed;