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

Fix self._handlers popping unknown controller ids #9

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

Conversation

kapitanluffy
Copy link

@kapitanluffy kapitanluffy commented Feb 13, 2024

I was experiencing this earlier so I thought it would be a nice quick fix.

Also, noticed that there's also an open issue #7. This PR should fix that.

@rchl
Copy link
Member

rchl commented Feb 13, 2024

But are you just hiding the root issue this way? The idea is that controller is responsible for unregistering itself. If that doesn't happen then there might be a deeper issue elsewhere.

That said, maybe it should just be a warning...

If you are developing a package then saving a plugin file triggers a reload that probably can cause such issue but it's sort of an "undefined" case since this is not quite a normal case.

@kapitanluffy
Copy link
Author

I do get that it is the controller's responsibility for unregistering itself but the FileWatcherChokidar class should be able to handle missing handler references instead of crashing.

So yeah, I guess a warning will do.

@rchl
Copy link
Member

rchl commented Feb 14, 2024

Crash shouldn't really affect anything besides producing a stack trace. A warning would still produce an output, only smaller. So I'm not sure there is a big difference there in terms of people being concerned/annoyed.

Can you specify how exactly it can be reproduced? We need to understand what happens and try to address the root issue. As I said in another issue, just ignoring this might result in file watcher never stopping until ST restart.

@kapitanluffy
Copy link
Author

We can manually reproduce it by reloading the FileWatcher package that I recently uploaded. The instances of handlers are probably wiped after the plugin got reloaded.

Other than that, I have seen it on my end but I do not know the cause. I doubt it was FileWatcher since I was not working on that at that time. That is why I am curious if the guys on issue #7 uses a package of mine.

I thought if it threw an error/exception, it will stop the code entirely. I noticed it on this line. That is why I added a check and allow it to call unregister regardless whether it is still in the _handlers or not

The other checks are when FileWatcher got reloaded.

Other thing I am thinking was if the reference is gone (Handler already deleted), we should remove it in _handlers.

@rchl
Copy link
Member

rchl commented Feb 14, 2024

The FIleWatcher package should handle unregistering of controllers when it's being reloaded. It can get notified on reloading by implementing a plugin_unloaded function.

@kapitanluffy
Copy link
Author

The FIleWatcher package should handle unregistering of controllers when it's being reloaded. It can get notified on reloading by implementing a plugin_unloaded function.

It does or maybe I am doing it wrong?

@kapitanluffy
Copy link
Author

The FIleWatcher package should handle unregistering of controllers when it's being reloaded. It can get notified on reloading by implementing a plugin_unloaded function.

It does or maybe I am doing it wrong?

@rchl here, I am calling this function on unload. Technically, it iterates over the handlers and calls destroy

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.

2 participants