-
Notifications
You must be signed in to change notification settings - Fork 177
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(core): CNX-8692 Fixed regression with AccountManager updating accounts while loading them from SQLite #3138
fix(core): CNX-8692 Fixed regression with AccountManager updating accounts while loading them from SQLite #3138
Conversation
@@ -257,8 +257,10 @@ public void UpdateObject(string hash, string serializedObject) | |||
{ | |||
CancellationToken.ThrowIfCancellationRequested(); | |||
|
|||
using var c = new SqliteConnection(_connectionString); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Our AccountManager does some potentially sketchy operations calling UpdateObject
while looping through the lazily enumerate return of GetAllObjects
. This causes GetAllObjects
to return forever, since it returns your updated objects also.
Reverting my change so GetAllObjects
and UpdateObject
use separate SQLite connections appears to fix things.
I'm a little concerned that we're actually relying on some sort of in-memory cache that the connections use.
I question if this is the behaviour we actually want, it seems to me the problem is with the way the AccountManager
is enumerating over a lazy enumerable that its updating. I'll open a ticket for next release to decide on what our desired behaviour here should be.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree. We should do some investigation as to why this is the way it works now, as this sounds super dodgy...
@@ -257,8 +257,10 @@ public void UpdateObject(string hash, string serializedObject) | |||
{ | |||
CancellationToken.ThrowIfCancellationRequested(); | |||
|
|||
using var c = new SqliteConnection(_connectionString); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree. We should do some investigation as to why this is the way it works now, as this sounds super dodgy...
d533c1e
to
d5d8396
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good! Live reviewed with Jedd
d5d8396
to
f915518
Compare
2dc7cea
to
4f423bb
Compare
…-queried-from-server
Merging in! Remember to open a ticket for the unresolved conversation above @JR-Morgan 🙌🏼 |
No description provided.