Skip to content

Conversation

gave92
Copy link
Member

@gave92 gave92 commented Nov 27, 2022

Resolved / Related Issues
Items resolved / related issues by this PR.

Validation
How did you test these changes?

  • Built and ran the app

@hez2010
Copy link
Member

hez2010 commented Nov 27, 2022

Even with LiteDB v4, it still does not work and causes crashing on multiple files instance startup. (Multiple processes cannot open the same LiteDB file simultaneously because the LiteDB will lock the opened file).
The crashing issue already exists before I bump the LiteDB to v5.

BTW the recommended way for concurrency is described here: https://github.com/mbdavid/LiteDB/wiki/Concurrency.

In first option (process safe), you will always work disconnected from the datafile. Each use will open datafile, lock file (read or write mode), do your operation and then close datafile.

So downgrading to v4 doesn't resolve any problem.

How about replacing LiteDB with SQLite?

@gave92
Copy link
Member Author

gave92 commented Nov 27, 2022

I can't reproduce the issue with v4, how do I trigger the crash? Simply opening a few instances of Files and browsing around appears ok. And it has always been like this since UWP days.
LiteDB does not lock the file with shared connection mode.

image

SQLite is even worse for concurrency iirc (throws the "database is locked" error all the times).

@hez2010
Copy link
Member

hez2010 commented Nov 27, 2022

Try opening two files instances and then do file operation (such as Copy, Move, Delete...) on one.

@hez2010
Copy link
Member

hez2010 commented Nov 27, 2022

The problem was here before we bump the LiteDB: #10008 (review)

@gave92
Copy link
Member Author

gave92 commented Nov 27, 2022

Have you verified with this PR? Code is a bit different from #10008.

Try opening two files instances and then do file operation (such as Copy, Move, Delete...) on one.

Appears ok for me

@hez2010
Copy link
Member

hez2010 commented Nov 27, 2022

I remembered that after merging #10008 we were not able to do the above things until #10250 got merged. Maybe something has changed later.
I think we need to test the LiteDB manually (maybe somewhere other than Files) to make sure a dbfile can be opened and operated from different processes simultaneously without any issue.

@gave92
Copy link
Member Author

gave92 commented Nov 27, 2022

I remembered that after merging #10008 we were not able to do the above things until #10250 got merged. Maybe something has changed later.

I'll take this as "I've tested your PR and it does not crash so it's worth more testing" :)

I think we need to test the LiteDB manually (maybe somewhere other than Files) to make sure a dbfile can be opened and operated from different processes simultaneously without any issue.

Good idea 👍

@gave92
Copy link
Member Author

gave92 commented Nov 27, 2022

Here is a console program that runs a bunch of read/write to the DB. Launching the exe a few times concurrently appears OK. Let me know if you want to run more tests.
LiteDBTest.zip

@hez2010
Copy link
Member

hez2010 commented Nov 27, 2022

Here is a console program that runs a bunch of read/write to the DB. Launching the exe a few times concurrently appears OK. Let me know if you want to run more tests. LiteDBTest.zip

Well seems that the concurrency issue no longer persists. It's good to me.

@gave92 gave92 marked this pull request as ready for review November 27, 2022 13:01
@gave92 gave92 requested a review from hez2010 November 27, 2022 13:01
@gave92
Copy link
Member Author

gave92 commented Nov 27, 2022

Wow I've just tried the test app with LiteDB v5 and read/write perfo is awful when using Shared mode (1 second per each write). That is probably the cause of slow loading of tags. Yuk LiteDB v5! (not their fault, I think they switched from file locks to global mutexes for concurrency).

Copy link
Member

@hez2010 hez2010 left a comment

Choose a reason for hiding this comment

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

LGTM.

@gave92 gave92 added ready to merge Pull requests that are approved and ready to merge and removed needs - code review labels Nov 27, 2022
@yaira2 yaira2 changed the title Fix: File tags are slow to load Fix: Fixed issue where tags loaded slowly Nov 27, 2022
@yaira2 yaira2 merged commit 886e170 into files-community:main Nov 27, 2022
@gave92 gave92 deleted the dblite_mess branch November 27, 2022 15:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready to merge Pull requests that are approved and ready to merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants