Skip to content
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

Jump to bottom

Allow calling initialize() only once #857

Merged
merged 2 commits into from
Jul 14, 2021
Merged

Allow calling initialize() only once #857

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c6a6aad
Allow calling initialize() only once
ilammy Jul 13, 2021
d722b42
Add this PR to the changelog reference list
ilammy Jul 14, 2021
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
2 changes: 1 addition & 1 deletion CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -154,7 +154,7 @@ _Code:_

- Updated Emscripten toolchain to the latest version ([#760](https://github.com/cossacklabs/themis/pull/760)).
- Node.js v16 is now supported ([#801](https://github.com/cossacklabs/themis/pull/801)).
- New initialization API: `initialize()`, allowing to specify custom URL for `libthemis.wasm` ([#792](https://github.com/cossacklabs/themis/pull/792), [#854](https://github.com/cossacklabs/themis/pull/854)).
- New initialization API: `initialize()`, allowing to specify custom URL for `libthemis.wasm` ([#792](https://github.com/cossacklabs/themis/pull/792), [#854](https://github.com/cossacklabs/themis/pull/854), [#857](https://github.com/cossacklabs/themis/pull/857)).

_Infrastructure:_

Expand Down
17 changes: 17 additions & 0 deletions src/wrappers/themis/wasm/src/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@

import context from "./context";
import libthemisFn from "./libthemis.js";
import { ThemisError, ThemisErrorCode } from "./themis_error";

export { SecureCellSeal } from "./secure_cell_seal";
export { SecureCellTokenProtect } from "./secure_cell_token_protect";
Expand Down Expand Up @@ -41,6 +42,8 @@ let onRuntimeInitialized: () => void
// is expected to call, then await the result to resolve before using
// other WasmThemis functions.

let libthemisInitialized = false;

/**
* Initialize WasmThemis.
*
Expand All @@ -54,15 +57,26 @@ let onRuntimeInitialized: () => void
*
* @param wasmPath URL of `libthemis.wasm` to download.
*
* @throws {ThemisError} is thrown if this function is called more than once,
* or if WasmThemis has been already initialized via `initialized`.
*
* @since WasmThemis 0.14.0
*/
export const initialize = async (wasmPath?: string) => {
if (libthemisInitialized) {
throw new ThemisError(
'initialize',
ThemisErrorCode.FAIL,
Copy link
Contributor

Choose a reason for hiding this comment

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

is explicit error really necessary?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

is explicit error really necessary?

Well, one approach is to immediately resolve initialize() successfully without actually doing anything if WasmThemis has been already initialized. However, I'd like to send a strong message that you should not call initialize() twice. If you do, there's likely some error in your code.

Copy link
Contributor

Choose a reason for hiding this comment

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

one approach is to immediately resolve initialize() successfully without actually doing anything if WasmThemis has been already initialized

i prefer this approach.

I'd like to send a strong message that you should not call initialize() twice. If you do, there's likely some error in your code.

but i also agree with you on this.

let's use your version :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Let's think about it like this: we can remove the error signaling and make initialize() idempotent in the future, if a genuine use case for that comes along. However, it would be a breaking change to make it an error to call initialize() twice, once we slap a note “It's totally okay to call this function multiple time with no consequences!”

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

So I'd rather make it an error now, before the users get to use it, and if they call it twice they get an error saying that it's an error. As opposed to them filing an issue, "I wanted to reinitialize with different settings, the function succeeded, but nothing changed", and us telling them, "well don't do that".

'WasmThemis can only be initalized once',
);
}
context.libthemis = await libthemisFn({
onRuntimeInitialized: () => onRuntimeInitialized(),
locateFile: wasmPath ? function () {
return wasmPath;
} : undefined,
});
libthemisInitialized = true;

return context.libthemis;
};
Expand Down Expand Up @@ -144,6 +158,9 @@ class InitializedPromise {
* in the same directory as the executing script.
* If you need to use a custom location, call `initialize()`.
*
* Note that you cannot use `initialize()` after WasmThemis has been initialized
* using this promise.
*
* @see initialize
*/
export const initialized = new InitializedPromise((resolve) => {
Expand Down
7 changes: 7 additions & 0 deletions src/wrappers/themis/wasm/test/initialization/initialize-path.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
const themis = require('../../src/index.ts')
const ThemisErrorCode = themis.ThemisErrorCode
const assert = require('assert')

describe('wasm-themis', function() {
Expand All @@ -16,5 +17,11 @@ describe('wasm-themis', function() {
done()
})
})
it('can only be initialized once', function(done) {
themis.initialize('src/libthemis.wasm').catch(function(e) {
assert.strictEqual(e.errorCode, ThemisErrorCode.FAIL)
done()
})
})
})
})
7 changes: 7 additions & 0 deletions src/wrappers/themis/wasm/test/initialization/initialize.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
const themis = require('../../src/index.ts')
const ThemisErrorCode = themis.ThemisErrorCode
const assert = require('assert')

describe('wasm-themis', function() {
Expand All @@ -10,5 +11,11 @@ describe('wasm-themis', function() {
done()
})
})
it('can only be initialized once', function(done) {
themis.initialize().catch(function(e) {
assert.strictEqual(e.errorCode, ThemisErrorCode.FAIL)
done()
})
})
})
})
8 changes: 8 additions & 0 deletions src/wrappers/themis/wasm/test/initialization/initialized.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
const themis = require('../../src/index.ts')
const ThemisErrorCode = themis.ThemisErrorCode
const assert = require('assert')

describe('wasm-themis', function() {
Expand All @@ -10,5 +11,12 @@ describe('wasm-themis', function() {
done()
})
})
// "themis.initialized" stays resolved but you cannot initialize again.
it('can only be initialized once', function(done) {
themis.initialize().catch(function(e) {
assert.strictEqual(e.errorCode, ThemisErrorCode.FAIL)
done()
})
})
})
})