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

Main+Store: NeDB persistence and integrity + dependency update #3845

Merged
merged 5 commits into from
Aug 10, 2023

Conversation

Svallinn
Copy link
Member

@Svallinn Svallinn commented Aug 3, 2023

Pull Request Type

  • Bugfix
  • Other

Related issue

I'm sure there are issues related to this, but I wouldn't close them until some time has passed in order to check if this passes the test of time (due to the integrity problem being such a rare heisenbug and dependent on timing).

Description

This PR replaces the nedb-promises dependency with @seald-io/nedb (which the former used internally).
Given that the internal dependency now supports Promises natively, nedb-promises is now fundamentally obsolete.

Additionally, clean up code in the main process has been freshen up and data store compaction is performed before terminating the main process to ensure that all former operations in the queue have finished executing.
This will hopefully prevent any integrity issues related to the datafiles in the case of standard process terminations.

Testing

Everything seems to be working correctly after the dependency switch and appropriate refactoring.

Desktop

  • FreeTube version: development (8025945)

@FreeTubeBot FreeTubeBot enabled auto-merge (squash) August 3, 2023 23:24
@github-actions github-actions bot added PR: dependencies Pull requests that update a dependency file PR: waiting for review For PRs that are complete, tested, and ready for review labels Aug 3, 2023
Copy link
Collaborator

@PikachuEXE PikachuEXE left a comment

Choose a reason for hiding this comment

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

Add some comments?

src/main/index.js Show resolved Hide resolved
src/datastores/handlers/base.js Show resolved Hide resolved
src/datastores/handlers/base.js Outdated Show resolved Hide resolved
Co-authored-by: PikachuEXE <pikachuexe@gmail.com>
@Svallinn
Copy link
Member Author

Svallinn commented Aug 4, 2023

Add some comments?

@PikachuEXE
IMO, anyone with IntelliSense or an internet connection can figure out what it does.
Comments like those should be for more obtuse developer-made code, not for standard built-in objects' methods.

PikachuEXE
PikachuEXE previously approved these changes Aug 5, 2023
@github-actions github-actions bot added PR: merge conflicts / rebase needed and removed PR: waiting for review For PRs that are complete, tested, and ready for review labels Aug 7, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Aug 7, 2023

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@github-actions
Copy link
Contributor

github-actions bot commented Aug 7, 2023

Conflicts have been resolved. A maintainer will review the pull request shortly.

@absidue absidue added the PR: waiting for review For PRs that are complete, tested, and ready for review label Aug 7, 2023
@efb4f5ff-1298-471a-8973-3d47447115dc

Leaving this for @absidue

@FreeTubeBot FreeTubeBot merged commit 079417d into development Aug 10, 2023
6 checks passed
@github-actions github-actions bot removed the PR: waiting for review For PRs that are complete, tested, and ready for review label Aug 10, 2023
@efb4f5ff-1298-471a-8973-3d47447115dc efb4f5ff-1298-471a-8973-3d47447115dc deleted the feat+fix/nedb-persistence branch August 10, 2023 20:47
@PikachuEXE
Copy link
Collaborator

PikachuEXE commented Aug 15, 2023

On MacOS (dev) when I quit the app it looks stuck for a long time (>10s)
Not sure how to display that it's doing stuff instead of stuck in something...

Edit: to reproduce, import subscriptions/clear subscriptions/playlist/history with large enough number (I did it with playlists on my playlist branch)

@Svallinn
Copy link
Member Author

On MacOS (dev) when I quit the app it looks stuck for a long time (>10s) Not sure how to display that it's doing stuff instead of stuck in something...

Edit: to reproduce, import subscriptions/clear subscriptions/playlist/history with large enough number (I did it with playlists on my playlist branch)

What's your basis on it appearing stuck? Genuinely asking since I don't know the MacOS details.
You couldn't have understood that from a window closing, since the compactation occurs after the windows are gone, that's why I'm asking.

Regardless, that also implies that, if you take those actions without my PR, these two things were probably happening before (depending on how activate event functions, these may be slightly off):

  • The datastores were never being compacted after the app ran the first time.
    My guess is that, when you kill all the windows, it still leaves the original datastore handlers in memory and just reuses them when you spawn a new window (on that same system boot obviously, rebooting gets rid of those handlers and it'll just compact on datastore load when opening the app on the next boot).
  • The prior point implies that there was probably a "delay" to process that large number of lines before as well, but it was only when there was no handlers to be reused and had to load new ones.
    Naturally, this would only happen if it's the very first time you open the app on that particular system boot, or one can probably force this somehow by killing Electron's main process for good and reopening the app.

These are, at best, speculations since I can't test, feel free to verify and let me know.
Also, "system boot" is probably not the right term for what I'm trying to transmit, but I think you can understand what I mean.

@PikachuEXE
Copy link
Collaborator

On MacOS one can quit an app by right clicking the icon on the dock
image

And with some conditions (probably when there are many changes to compact) the app would appear unresponsive (but it's actually compacting without any visual response)
So if user right clicks again it looks like this
image

I know FT is doing stuff but I guess most users don't know that
Just thinking if there is an easy way to NOT make FT look stuck and make user more likely to click on Force Quit

@Svallinn
Copy link
Member Author

Svallinn commented Aug 15, 2023

It's a tough situation to work around...
I still gotta say, 10 seconds feels like a fucking eternity even if you have 250 subscriptions removed or so.
Is NeDB that slow? Probably, but...
I hate to push this onto you, but can you remove these two from cleanUpResources in the main.js

session.defaultSession.clearCache(),
session.defaultSession.clearStorageData({
  storages: [
    'appcache',
    'cookies',
    'filesystem',
    'indexdb',
    'shadercache',
    'websql',
    'serviceworkers',
    'cachestorage'
  ]

build and check if that still happens?
Just wanna make sure it's NeDB before I even consider putting in the effort of thinking about this.

@PikachuEXE
Copy link
Collaborator

Let me see how to reproduce that long exit delay consistently first
But I never experiences this until 1-3 weeks ago
This PR is the only one that I can think of (as it alters exit logic)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: dependencies Pull requests that update a dependency file
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants