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

Sqlite wallet issue #299

Closed
dangershony opened this issue Mar 9, 2021 · 9 comments · Fixed by #300
Closed

Sqlite wallet issue #299

dangershony opened this issue Mar 9, 2021 · 9 comments · Fixed by #300

Comments

@dangershony
Copy link
Member

Change to RELEASE mode, run any node, create a wallet and query for address.

This will fail

@sondreb
Copy link
Member

sondreb commented Mar 9, 2021

image

@sondreb
Copy link
Member

sondreb commented Mar 9, 2021

The issue is many requests at the same time. If you request the balance in the browser and keep refreshing, it will quickly begin crashing.

Example: http://localhost:33333/api/wallet/balance?walletName=main&accountName=account%200

@MithrilMan
Copy link
Contributor

does SQLite doesn't support multiple access to db?
that would be horrible, and in this case the implementation should have a lock (a slim one would suffice)

@sondreb
Copy link
Member

sondreb commented Mar 9, 2021

Eventually when reloading, you'll get the error out like this:
image

@MithrilMan
Copy link
Contributor

is it based on Dapper?
ensure the context is scoped, not a singleton, because if it's like EF, you can't run concurrent operation on the same context

@MithrilMan
Copy link
Contributor

I checked fast, it seems dapper is used in WalletStore that is created once for each wallet, this mean it's a shared instance and since WalletStore create teh db connection in its constructor, that's the problem

in EF the dbcontext should be short living (like a unit of work), I suppose the same should be done on Dapper?

@dangershony
Copy link
Member Author

I think dapper opens and closes the connection internally, I am not sure it needs to be short lived, but that's a place to look yes.

@dangershony
Copy link
Member Author

But I am pretty sure the wallet has locks on it before even reaching the wallet store

@MithrilMan
Copy link
Contributor

it doesnt, look the method pointed out by @sondreb, you'll find a loop that calls address.GetBalances, if you follow it you'll see there are no locks

sondreb added a commit that referenced this issue Mar 9, 2021
- This is the smallest impact and quickest way to fix the issue with connections against sqlite database.
- Closes #299
sondreb added a commit that referenced this issue Mar 9, 2021
…ct (#300)

* Make sure that each query against database uses a new connection object

- This is the smallest impact and quickest way to fix the issue with connections against sqlite database.
- Closes #299

* fix memory conn

* Ensure all calls to GetDbConnection is wrapped with using clause

* Update version for release 20

* Minor additional improvements on connection usage

Co-authored-by: SondreB <sondre@outlook.com>
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 a pull request may close this issue.

3 participants