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.

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

Allow calling initialize() only once #857

Merged
merged 2 commits into from
Jul 14, 2021

Conversation

ilammy
Copy link
Collaborator

@ilammy ilammy commented Jul 13, 2021

WasmThemis initialization fills in the context.libthemis singleton that is actually used by all WasmThemis functions. That is, calling initialize() multiple times is technically permitted, but it might not have an effect that you expect. WasmThemis will always use WebAssembly context from the last initialization.

While reinitialization might be useful, it does not seem to be now. Let's allow calling initialize() only once. (This includes calling it indirectly via the initialized promise.) We can always allow it later, once the semantics of this are clarified.

Checklist

@ilammy ilammy added the W-WasmThemis 🌐 Wrapper: WasmThemis, JavaScript API, npm packages label Jul 13, 2021
@ilammy ilammy requested review from vixentael and a team July 13, 2021 08:12
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".

Copy link
Contributor

@vixentael vixentael left a comment

Choose a reason for hiding this comment

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

In terms of changes that affect users: these are breaking changes, users will need to change initialize() to initialized across their code (in one place typically).

Right?

WasmThemis initialization fills in the "context.libthemis" singleton
that is actually used by all WasmThemis functions. That is, calling
initialize() multiple times is technically permitted, but it might not
have an effect that you expect. WasmThemis will always use WebAssembly
context from the last initialization.

While reinitialization might be useful, it does not seem to be now.
Let's allow calling initialize() only once. (This includes calling it
indirectly via the "initialized" promise.) We can always allow it later,
once the semantics of this are clarified.
@ilammy ilammy force-pushed the typescript-initialize-once branch from 62fb719 to d722b42 Compare July 14, 2021 13:16
@ilammy ilammy marked this pull request as ready for review July 14, 2021 13:16
@ilammy
Copy link
Collaborator Author

ilammy commented Jul 14, 2021

The base branch got merged and GitHub does not really support stacked PRs all that well. I have rebased the branch (with no changes), and throw an extra commit in with a line in the changelog.

@ilammy
Copy link
Collaborator Author

ilammy commented Jul 14, 2021

@vixentael,

these are breaking changes, users will need to change initialize() to initialized across their code (in one place typically)

Not really. initialize() has not been released yet. It's a new thing that's going to appear in WasmThemis 0.14.0. Users of 0.13.X have only initialized and they can continue using it just fine.

@ilammy ilammy merged commit e114ad9 into cossacklabs:wasm-typescript Jul 14, 2021
@ilammy ilammy deleted the typescript-initialize-once branch July 14, 2021 13:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
W-WasmThemis 🌐 Wrapper: WasmThemis, JavaScript API, npm packages
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants