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

Do not execute background runner if device is suspended #3608

Merged
merged 4 commits into from Jan 17, 2018

Conversation

Projects
None yet
3 participants
@Hzj-jie
Contributor

Hzj-jie commented Jan 16, 2018

On kindle voyage, the background runner may still execute a while even device has been suspended. Because kindle stock software may wake up the device to update itself. The behavior of the stock software impacts KOReader. For example, the AutoFrontlight plugin may turn on the frontlight unexpectedly.

@@ -597,4 +597,9 @@ function util.shell_escape(args)
return table.concat(escaped_args, " ")
end
table.clear = function(t)
local c = #t
for i = 0, c do t[i] = nil end

This comment has been minimized.

@Frenzie

Frenzie Jan 16, 2018

Member

for i in next, t do t[i] = nil end?

This comment has been minimized.

@Frenzie

Frenzie Jan 16, 2018

Member

for k,v in pairs(t) do t[k] = nil end?

This comment has been minimized.

@Hzj-jie

Hzj-jie Jan 16, 2018

Contributor

According to this thread, https://stackoverflow.com/questions/4880368/how-to-delete-all-elements-in-a-lua-table, this is the fastest way to clear a table.

This comment has been minimized.

@Frenzie

Frenzie Jan 16, 2018

Member

I was talking about legibility since it doesn't really feel like Lua style,[1] but if we're concerned about performance we should completely ignore Lua benchmarks and profile LuaJIT.

[1] Hm, I hope I'm not sounding too much like one of those people who's always saying something or other isn't "Pythonesque".

@@ -597,4 +597,9 @@ function util.shell_escape(args)
return table.concat(escaped_args, " ")
end
table.clear = function(t)

This comment has been minimized.

@Frenzie

Frenzie Jan 16, 2018

Member

Should we act as if it's a language primitive?

@poire-z

This comment has been minimized.

Contributor

poire-z commented Jan 16, 2018

Not really related to this PR code, but as I see you have added ways for starting/stoping the backgroundrunner:
I would be really grateful if you could make BackgroundRunner not start until it's needed. Like: other code that just added a job to PluginShare.backgroundJobs could brocastEvent("EnsureBackgroundRunnerRunning"), and you would in that BackgroundRunnerWidget:onEnsureBackgroundRunnerRunning() start it if not already running. And elsewhere have it stop itself when the job list gets empty.
For now, it seems only necessary on a few kindle devices for the autofrontlight plugin.
All other devices get it running on startup for no use, and it spams the emulator console log, and the android logcat (when koreader is in the background, when you wake up your phone to take a call, backgroundrunner just runs :)

@@ -597,4 +597,9 @@ function util.shell_escape(args)
return table.concat(escaped_args, " ")
end
table.clear = function(t) -- luacheck: ignore 122

This comment has been minimized.

@Frenzie

Frenzie Jan 16, 2018

Member

I was going to complain about setting a fake language primitive myself actually; I hadn't realized luacheck had flagged it already.

This comment has been minimized.

@Frenzie

Frenzie Jan 16, 2018

Member

Oh wait, I did complain about it. :-P

This comment has been minimized.

@Hzj-jie

Hzj-jie Jan 17, 2018

Contributor

Oh, I thought table.clear() was preferred. If not, I will move it to util.

@Hzj-jie

This comment has been minimized.

Contributor

Hzj-jie commented Jan 17, 2018

I have a change (master...Hzj-jie:master) to move network polling logic into background runner to avoid up to 1 second delay when navigating to the network menu (

function NetworkMgr:isOnline()
). But unfortunately, I have not finished it yet. With this change, background runner should be more useful on other platforms.

The problem of starting or stopping background runner is that it's a plugin rather than a core feature. Accessing it can only be handled through PluginShare, which is just a table. Lua does not have signal-event mechanism to notify background runner when a new job has arrived.

But if the concept is proved to be workable, we can move the logic into core, and make improvements to the scheduling.

@Frenzie Frenzie merged commit 65f26ce into koreader:master Jan 17, 2018

1 check passed

ci/circleci Your tests passed on CircleCI!
Details
@poire-z

This comment has been minimized.

Contributor

poire-z commented Jan 17, 2018

Accessing it can only be handled through PluginShare, which is just a table.
Lua does not have signal-event mechanism to notify background runner when a new job has arrived.

Lua may not have that, but KOReader has it with its sendEvent/broadcastEvent and the onEvent methods.

But hey, why not just use the PluginShare you introduced so that plugins could make available thru it some of their services by plugging function into PluginShare (ie: not just use for passing/consuming data, but also for exposing services - this would fit into its name and philosophy :)

Like in backgroundrunner.koplugin/main.lua you could do

PluginShare.addBackgroundRunnerJob = function(job)
    BackgroundRunner:_insertAndStartIfNeeded(job)
end

Clients would then not need to know about the internal PluginShare.backgroundJobs and just use:

if PluginShare.addBackgroundRunnerJob then -- the service is available
    PluginShare.addBackgroundRunnerJob( {my_job_specs} )
end

(Even with your other network polling PR, it may make sense to remove the job of pinging every N seconds when you know wifi is disabled, and re-add it when wifi is enabled - and to not have backgroundrunner checking every 2 seconds that its job list is empty - once it's empty, it won't be filled until BackgroundRunner:_insertAndStartIfNeeded(job) is called again)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment