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

thread safe extra network list_items #13014

Merged
merged 5 commits into from
Nov 19, 2023

Conversation

w-e-w
Copy link
Collaborator

@w-e-w w-e-w commented Sep 3, 2023

Description

should prevent this from happening #13011 #13896
I was able to reproduce the issue using strategically placed breakpoints manually steping

since refreshing of extra networks can be called concurrently
the dictionary might be modified during looping list_items
we should use locking to protect it against accidental unexpected changes

even though the error described in the issue post is only for lora
as other extra networks tab share the same logic I applyed the same treatment to all of them

Checklist:

@catboxanon catboxanon linked an issue Sep 5, 2023 that may be closed by this pull request
1 task
@AUTOMATIC1111
Copy link
Owner

There is #12601 and 35db366 as a fix so maybe the same solution can be applied here.

@w-e-w
Copy link
Collaborator Author

w-e-w commented Sep 9, 2023

when I was working on this I did consider also locking list_models()
but as far as I can tell only list_items(self) needs to be atomic

the worst that can happen list_models() if it's not atomic is that the final display list may be incomplete
but means that they are multiple requests of refresh models
they first request due to the second request might have the listing incomplete
but the second request will quickly correct the results

if you prefer we also lock list_models() I could apply it

@AUTOMATIC1111
Copy link
Owner

My preference is no locks.

@w-e-w
Copy link
Collaborator Author

w-e-w commented Sep 9, 2023

ah I see let me check if 35db366 that works
I thought it was the same link as #12601

@w-e-w
Copy link
Collaborator Author

w-e-w commented Sep 9, 2023

nope
I managed to broke it
in the case of 35db366

it was create_item() that broke

def create_item(self, name, index=None, enable_filter=True):
checkpoint: sd_models.CheckpointInfo = sd_models.checkpoint_aliases.get(name)
path, ext = os.path.splitext(checkpoint.filename)

caused by

def list_models():
checkpoints_list.clear()
checkpoint_aliases.clear()

checkpoint_aliases.clear() was cleared and so sd_models.checkpoint_aliases.get(name) return None

I used .get(name, None) to fix this and if so early return None, after that only yield if not None

so in the rare event that this actually happeneds
it will fail to display that particular card
but it should be automatically resolved by the second refresh call (which was the cause of the issue)

if you're in a multi-user scenario there is a higher chance of breaking and it won't automatically fix because the second refresh is from a different user so the current user won't receive the good update
but I think the chance is already low enough that we don't need to care at least for now
hopefully....

extensions-builtin/Lora/ui_extra_networks_lora.py Outdated Show resolved Hide resolved
modules/ui_extra_networks_checkpoints.py Outdated Show resolved Hide resolved
modules/ui_extra_networks_hypernets.py Show resolved Hide resolved
modules/ui_extra_networks_checkpoints.py Outdated Show resolved Hide resolved
modules/ui_extra_networks_hypernets.py Outdated Show resolved Hide resolved
modules/ui_extra_networks_textual_inversion.py Outdated Show resolved Hide resolved
@w-e-w w-e-w marked this pull request as draft September 11, 2023 09:14
@w-e-w w-e-w force-pushed the thread-safe-extranetworks-list_items branch from af4a03b to e785402 Compare September 11, 2023 11:00
@w-e-w w-e-w marked this pull request as ready for review September 11, 2023 11:07
@w-e-w
Copy link
Collaborator Author

w-e-w commented Sep 11, 2023

oops
accidentally click request review

@w-e-w
Copy link
Collaborator Author

w-e-w commented Sep 12, 2023

I have a suspicion that the issue caused by .get(name) is it extreme rare case that when the
get is called and the key is found, but after the key is found and it's trying to access it the entire dictionary got modified
and so the .get(name) managed to generate a key not found error

I suspect one would have to be extremely unlucky for this to happen

it can also be an issue triggered by debugger

@w-e-w w-e-w mentioned this pull request Nov 11, 2023
4 tasks
@AUTOMATIC1111 AUTOMATIC1111 merged commit 337bc4a into dev Nov 19, 2023
6 checks passed
@AUTOMATIC1111 AUTOMATIC1111 deleted the thread-safe-extranetworks-list_items branch November 19, 2023 06:09
@w-e-w w-e-w mentioned this pull request Dec 4, 2023
@w-e-w w-e-w mentioned this pull request Dec 16, 2023
CastielMa added a commit to CastielMa/stable-diffusion-webui that referenced this pull request May 26, 2024
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.

[Bug]: Runtime error dictionary changed bla bla
3 participants