Cmm crash fix #478
Merged
Cmm crash fix #478
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…ols has been deleted’ To reproduce: - Open kicad-jlcpcb-tools dialog - Close dialog - Re-open kicad-jlcpcb-tools dialog - Attempt to download the database (this causes log messages to be generated), see RuntimeError dialog pop up. Each time the mainwindow dialog is opened, init_logger() is called. init_logger() retrieves the root logger via a call to logging.getLogger(). Handlers are then attached to the root logger, one to log to stderr and one to log via the LogBoxHandler class to the wxWidgets textctrl log box at the bottom of the mainwindow dialog. Recently there was a change to use wxPython events to log messages, to fix an issue that was believed to be due to manipulation of the log output textctrl widget from the backgound thread that was downloading the database. This change removed a try/except that was discarding exceptions, as it was believed that this change fixed the cause of the exceptions by marshalling, via wx.queueEvent(), the log text to the main thread for addition to the log output textctrl widget. Root cause is that the Python instance persists for the duration of Kicad's execution. Thus calls to addHandler() to the root logger are cumulative. Each time you re-open the mainwindow the logging handlers are added. The previous handlers, which now refer to deleted instances of the mainwindow (class JLCPCBTools, see the error message in the commit subject), persist. Each subsequent call to Python's logging functions results in all of these loggers being called. Previously the try/except in each of these loggers was discarding the wxPython/wxWidgets errors but removing the try/except meant they were now unhandled. Fix the crash by calling removeHandler() in quit_dialog() so there are no old logger handlers with references to now deleted dialogs. Note that due to the try/except it is likely that this issue was latent in the plugin for months or years, at least since the addHandler() was called without removeHandler(). Special thanks to Aleksandr Shvartzkop took time to look at the code and almost immediately spotted the logger addHandler() calls without corresponding removeHandler() calls. This was the first step that led to debugging the issue.
|
Installed the fix and downloaded all jclpbc parts. Everything seems to be back to normal now (kicad 8.0.2 under Manjaro). Thank you so much for fixing the issue! |
|
@Bouni can you review and merge if this looks good? Would like to get this out and released since it's affecting most users. |
Bouni
approved these changes
May 29, 2024
|
Thanks @chmorgan for digging into this. The log function needs an overhaul anyway. I've stolen it from KiBuzzard plugin and it has sometimes strange behavior. I would really like it if we could write to a log file as well (maybe that can be enabled in the settings!?) |
|
I think the odd behavior is fixed with this pr and imo it’s necessary to
marshal the events due to widgets threading restrictions, or maybe I’m
wrong here?
Agree on the log file. That could help a ton with debugging issues.
…On Wed, May 29, 2024 at 10:43 AM bouni ***@***.***> wrote:
Thanks @chmorgan <https://github.com/chmorgan> for digging into this.
The log function needs an overhaul anyway. I've stolen it from KiBuzzard
plugin and it has sometimes strange behavior. I would really like it if we
could write to a log file as well (maybe that can be enabled in the
settings!?)
What do you think?
—
Reply to this email directly, view it on GitHub
<#478 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAJH4AFK6AFMK6YCSFFYWY3ZEXSS5AVCNFSM6AAAAABIKJVA2CVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCMZXGU4TGNBYGA>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.