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

Refactor: startup process refactoring for manager #406

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

datawhores
Copy link
Collaborator

  • create a transitionmanager startup process
  • remove async_startup
  • add reset function if actions need to be run for each config, since startup only runs once

@NTFSvolume
Copy link
Collaborator

This could break a lot of things. It may work as long as no unknown exception are raised. There also a lot of functions that, even though they are synchronous, they use asyncio.run under the hood and they will throw an error cause a loop is already running. I think the changelog query uses it, the UI hash helper uses it and the cache clear option uses it.

It's better to just add more robust handling of exceptions inside the synchronous startup

@NTFSvolume
Copy link
Collaborator

We can prevent all of this by deprecating transfer_v4_to_v5. CDL will no longer try to transfer a v4 config at startup. The user can still do it but the have to use the UI option.

@jbsparrow
Copy link
Owner

Realistically, I don't think we need to keep the transfer function. We're releasing v6 and keeping the v4 code seems unnecessary.

If anyone is still using v4, we can have the transfer thing say to downgrade to the last version that supports v4 upgrades, then move to v6.

Would simplify the code for us and remove what is in most cases an unnecessary option.

Another option could be to have a script that handles the transfers?

@datawhores
Copy link
Collaborator Author

datawhores commented Dec 27, 2024

There's a v4 to v5 transfer function
And basically a v5 to v6 transfer function

I'm not sure why people don't have the hash table, it's been in the code for a bit now. But there's no check for the history table for v4 to v5 function, and the create history table function isn't called until later. That transfer seems to run okay

@NTFSvolume
Copy link
Collaborator

I'm not sure why people don't have the hash table, it's been in the code for a bit now. But there's no check for the history table for v4 to v5 function, and the create history table function isn't called until later. That transfer seems to run okay

The reason they do not have a hash table is cause the database is completely empty (0B). It doesn't have any table but the file does exists. Right now the only condition to start the v5 to v6 transfer is that the file exists and that's why it fails.

def transfer_v5_to_new_hashtable(self):
"""
transfers from old v5 hash table to new v5 hash table, that supports multiple hash types per file
"""
db_path = constants.APP_STORAGE / "Cache" / "cyberdrop.db"
if db_path.exists():
transfer_from_old_hash_table(db_path)

The database being a 0B file happens if the user starts CDL with the UI but exits immediately without doing any download, like mentioned #409. That means the normal startup finished (created the AppData folder and all the files), but the async_startup did not (create and pre allocate the database)

That's why i think #407 is a more proper fix cause it just stop the transfer if the table does not exists. We don't need to transfer the database to v6 cause it will already have the v6 schema when it gets created later on in the logic.

@NTFSvolume
Copy link
Collaborator

Realistically, I don't think we need to keep the transfer function. We're releasing v6 and keeping the v4 code seems unnecessary.

If anyone is still using v4, we can have the transfer thing say to downgrade to the last version that supports v4 upgrades, then move to v6.

Would simplify the code for us and remove what is in most cases an unnecessary option.

Another option could be to have a script that handles the transfers?

I agree but lets wait until the next release. I would like to test v4 import after all the other problems are fixed, to make sure it works and we have a correct reference of the last version that supported v4.

After that we can remove it and then move the v5 to v6 import from the normal startup to the async_startup cause nothing depends on the database until the actual scraping begins.

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