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

Catch and handle errors if getOptionsFull fails #930

Merged
merged 1 commit into from
May 14, 2024

Conversation

Kuuuube
Copy link
Member

@Kuuuube Kuuuube commented May 13, 2024

Fixes #723

I think this issue is due to calling triggerDatabaseUpdated without an await before _addDictionarySettings causing a race condition. But due to the nature of this bug there's no way to be 100% sure.

Simply awaiting triggerDatabaseUpdated doesn't work due to it calling ensureDictionarySettings conflicting with _addDictionarySettings and creating duplicate dict settings. (ensureDictionarySettings makes sure all dicts have settings entries and adds one if it is lacking. _addDictionarySettings adds a setting regardless of whether one exists already.)

Running triggerDatabaseUpdated after _addDictionarySettings appears to work perfectly fine.

I've also allowed getOptionsFull to retry up to 10 times if it fails, catching any errors so the dict import process is not interrupted.

At a minimum these fixes will make it so firefox users don't have to worry about the dict import process randomly breaking when importing multiple dicts and requiring manually restarting the import. But hopefully this will entirely eliminate settings related errors for firefox imports.

@Kuuuube Kuuuube added kind/bug The issue or PR is regarding a bug browser/firefox The issue is Firefox-only labels May 13, 2024
@Kuuuube Kuuuube requested a review from a team as a code owner May 13, 2024 18:21
@Kuuuube Kuuuube added the priority/high High priority issue/PR label May 13, 2024
@Kuuuube Kuuuube added this to the Next Release milestone May 13, 2024
@jamesmaa
Copy link
Collaborator

Why would getOptionsFull be flaky?

@Kuuuube
Copy link
Member Author

Kuuuube commented May 14, 2024

Not sure, it seems to be fine elsewhere but there's been a decent amount of reports about it breaking in this particular spot and I was able to replicate it.

This also only happens on firefox so it's probably safe to say it's a browser related bug. Easy enough to work around on our end though.

@Kuuuube
Copy link
Member Author

Kuuuube commented May 14, 2024

Also could be something related to updating the database right before fetching options. There arent many places that happens.

@jamesmaa jamesmaa added this pull request to the merge queue May 14, 2024
Merged via the queue into yomidevs:master with commit e73e081 May 14, 2024
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
browser/firefox The issue is Firefox-only kind/bug The issue or PR is regarding a bug priority/high High priority issue/PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Dictionary importing occasionally ends with error
2 participants