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

fix: race conditions #2218

Merged
merged 8 commits into from
May 2, 2023
Merged

fix: race conditions #2218

merged 8 commits into from
May 2, 2023

Conversation

reneaaron
Copy link
Contributor

@reneaaron reneaaron commented Mar 15, 2023

Describe the changes you have made in this PR

First steps for fixing the issue @BlowaterNostr brought up in #2182:

I have found the reason and it's a good indicator that Alby should support concurrent API calls or at least throw errors when user concurrently calls.

The story is this, in Blowater, we concurrently call encrypt/decrypt/signEvent/getPublicKey in many different promise chains. The current behavior of window.nostr is to simply return the result of the first call in the current JS tick. For example, if

getPublicKey()
encrypt()

are called in the same tick. await encrypt() will return the public key. Although in the log Alby gives a warning message, but the code behavior is not a good design. I would categorize it as either a bug or a design flaw.

I worked around this problem by adding a queue in my application code to wait for the previous call to resolve before invoking the next.

But, because Blowater also use IndexedDB, therefore, wrong data (should be an event, but was {enabled: true}) before I added the queue were persisted. I happened to never delete the database on Firefox, so that only Firefox appears buggy. I deleted the database and everything works now.

Fundamentally, TS is not type safe when it comes to serialize raw JSON to objects. Therefore, if application code always do type check during de serialization at runtime, this bug can be prevented. But this is not the norm in TS/JS code.

The communicate between extension and application is similar to serialization because it's also across program boundaries. We need better type safety here.

On Alby side, it should not return incorrect data (silent error) even if concurrent call is not allowed. Throwing exceptions is acceptable if concurrent call needs more time to design.

I've also put together a little repro case to test this:
https://codepen.io/reneaaron/pen/zYJjZbW

So currently some of the calls will still fail as we currently only allow one call at a time, however the results always match the call as now the promises are resolved for the executing call.

@github-actions
Copy link

github-actions bot commented Mar 15, 2023

🚀 Thanks for the pull request!

Here are the current build files for testing:

Download and unzip the file for your browser. Refer to the readme for detailed install instructions.


This build is brought to you by: channel.ninja (who recently dropped 1000 sats):


Want to sponsor the next build? send some sats to ⚡️builds@getalby.com (don't forget to provide your name)

Don't forget: keep earning sats!

@reneaaron reneaaron marked this pull request as ready for review April 11, 2023 09:54
@reneaaron reneaaron requested a review from rolznz April 13, 2023 10:16
@bumi bumi merged commit 475221e into master May 2, 2023
@bumi bumi deleted the fix/race-condition branch May 2, 2023 08:32
@reneaaron reneaaron mentioned this pull request May 2, 2023
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants