plugin handlers traceback - #13677
Conversation
Maybe @hius07 can help? |
02e89cd to
142d42d
Compare
| -- and the second is the call method of HandlerSandbox. | ||
| logger.err("An error occurred while executing a handler:\n" ..err .. "\n" .. debug.traceback(self.context.name .. ":" .. self.fname, 2)) | ||
| end | ||
| local ok, re = xpcall(self.f, traceback, self.context, unpack(arg)) -- unpack arg table into arguments |
There was a problem hiding this comment.
Anything wrong with keeping a clean variadic ... forward?
As is, this breaks if there are nils in the arguments (unpack can help with that, but only when using the three-argument form, where you pass the start & end indices, c.f., frontend/ui/widget/eventlistener.lua).
There was a problem hiding this comment.
Was not aware of the variadic forwarding. Is this a LuaJIT extension? I'll update to use that.
|
Running with kodev actually surfaces more errors. Most are I'm not sure why these were not reported when using The most likely culprit is my attempt at backward compatibility. In The plugins I saw define handlers as methods of the module table and therefore I need to pass the module table itself as first argument by referencing Is there a way to attach a debugger to koreader as is? Here the additional tracebacks from the various plugins. |
|
You can use https://github.com/pkulchenko/MobDebug?tab=readme-ov-file#usage in ZeroBrane |
|
I'm not sure I understand what these stacktraces you get are. For what's it's worth, I have used xpcall in frontend/ui/trapper.lua and plugins/httpinspector.koplugin/main.lua. |
I get them after wrapping and there was no error before. I will try running with the debugger attached so I can see if it's my fault.
Thanks for the pointers. |
|
From what I see in the stacktraces, all the mess happens when indexing |
In a previous revision I would not pass the module as |
|
Poking around with the debugger I can now say the following: the handlers may be fired before the plugin is initialized. On my laptop I can reliably trigger handlers for the AutoDim plugin by clicking around, this happens before the The same explanation goes for the rest of the errors that I see (I hope?). I'm not sure what the lifecycle of the plugin is, e.g. when/where |
|
That said, the handlers should only actually get reached by Events (as, as such, fired) after they've been "tied" to a top-level widget. In case of plugins, that's specifically either ReaderUI or FileManager, via Unless you've got stale instances of previous ReaderUI or FM instances (despite the following, somehow?), and its the teardown of ReaderUI/FM (that has a framework in place to orphan plugin instances, via |
|
We take great pains to avoid this sort of shenanigans happening (and both FM/RD themselves should shout very loudly if they seem to think it is happening on init), but it is fairly easy to break, and it has been horribly broken in the past, so, check which instance of the main "app" the broken stuff is actually tied to (i.e., what the (And, for that matter, which instances of Plugins themselves are actually broken). |
I was passing the plugin module as self in place of the plugin module instance. Doh! I am now happy with the outcome, I have stacktraces on event handlers. I think it should be easy to use xpcall everywhere in this file with the same approach and get stacktraces at load time too. I will take a look at frontend/ui/trapper.lua and plugins/httpinspector.koplugin/main.lua to see if I can steal something wrt choosing xpcall or pcall at runtime |
|
Is there a way to run the checks from the CI locally? |
|
Which parts? For luacheck: |
Thanks. I think this will be enough. Most of the CI builds are because I failed to retab the file so this should fix the rest. |
| return setmetatable(t, HandlerSandbox.mt) | ||
| end | ||
|
|
||
| function HandlerSandbox:call(module,...) |
There was a problem hiding this comment.
über-nit: Missing space after the comma ;).
| -- self parameter of the handlers | ||
| local ok, re | ||
| if self.log_stacktrace then | ||
| local traceback = function (err) |
There was a problem hiding this comment.
über-nit bis: no space between the function keyword and the parens
Do you want the toggling to be available at runtime? Right now it's done at load-time but I don't see a developer options menu. If you can point me towards it I can add the toggling at runtime. |
It's available only in File browser (not in Reader). koreader/frontend/apps/filemanager/filemanagermenu.lua Lines 523 to 809 in 48d50f7
Load-time is fine. When one is testing some plugin, he will restart koreader often enough :) |
|
I am done and happy with the result. I will submit a second PR for errors when loading plugins. |
|
@NiLuJe any comment on the plugin handlers sandbox that I can address? |
|
No time to review this seriously for a couple of weeks, sorry ;).. |
6 participants
Errors in plugins are logged but they are not actionable. As a plugin developer I expect some help from koreader.
This patch provides a way to get a lua stack trace when a plugin event handler raises an error. This is achieved by wrapping each event handler with
xpcallin place ofpcall. This lua builint allows executing a error handler before the stack is unwound. The traceback we get is then from the execution in the plugin context.The following is an example of where I am stuck. These are the last two errors I got. I am not sure if I have uncovered known errors or I am doing something wrong in the wrapping.
I made this patch by unpacking the appimage but I will transition to kodev soon to test more.
This change is