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

Dict lookups: have them always interruptible #5173

Merged
merged 2 commits into from Aug 3, 2019

Conversation

@poire-z
Copy link
Contributor

commented Aug 3, 2019

They should be now interruptible when fuzzy search is disabled, and on Android.
See #5151 and #3588 (comment).
Closes #5151.
(Tested on my Android 6 phone, hope they work as well on other android versions, with hopefully correct shells.)

Also increase the timeout when downloading Wikipedia HTML, needed when making an EPUB out of https://zh.wikipedia.org/wiki/%E4%B8%AD%E5%8D%8E%E4%BA%BA%E6%B0%91%E5%85%B1%E5%92%8C%E5%9B%BD).

poire-z added some commits Aug 3, 2019

Dict lookups: have them always interruptible
They should be now interruptible when fuzzy search is disabled
and on Android.

@Frenzie Frenzie added this to the 2019.08 milestone Aug 3, 2019

@NiLuJe

This comment has been minimized.

Copy link
Member

commented Aug 3, 2019

Is the pipe closed on abort?

I can somewhat reliably break subsequent dictionary lookups by repeatedly cancelling early on a PW2.

@poire-z

This comment has been minimized.

Copy link
Contributor Author

commented Aug 3, 2019

Is the pipe closed on abort?

Yes, it should, we do that correctly I think:

local std_out = io.popen(cmd, "r")
if std_out then
-- We check regularly if data is available to be read, and we give control
-- in the meantime to UIManager so our trap_widget's dismiss_callback
-- get a chance to be triggered, in which case we won't wait for reading,
-- We'll schedule a background function to collect the uneeded output and
-- close the pipe later.
while true do
-- Every 10 iterations, increase interval until a max of 1 sec is reached
check_num = check_num + 1
if check_interval_sec < 1 and check_num % 10 == 0 then
check_interval_sec = math.min(check_interval_sec * 2, 1)
end
-- The following function will resume us at coroutine.yield() below
-- with a go_on = true
local go_on_func = function() coroutine.resume(_coroutine, true) end
UIManager:scheduleIn(check_interval_sec, go_on_func) -- called in 100ms by default
local go_on = coroutine.yield() -- gives control back to UIManager
if not go_on then -- the dismiss_callback resumed us
UIManager:unschedule(go_on_func)
-- We forget cmd here, but something has to collect
-- its output and close the pipe to not leak file handles and
-- zombie processes.
local collect_and_clean
collect_and_clean = function()
if ffiutil.getNonBlockingReadSize(std_out) ~= 0 then -- cmd started outputing
std_out:read("*all")
std_out:close()
logger.dbg("collected cancelled cmd output")
else -- no output yet, reschedule
UIManager:scheduleIn(collect_interval_sec, collect_and_clean)
logger.dbg("cancelled cmd output not yet collectable")
end
end
UIManager:scheduleIn(collect_interval_sec, collect_and_clean)
break
end
-- The go_on_func resumed us: we have not been dismissed.
-- Check if pipe is ready to be read
if ffiutil.getNonBlockingReadSize(std_out) ~= 0 then
-- Some data is available for reading: read it all,
-- but we may block from now on
output = std_out:read("*all")
std_out:close()
completed = true
break
end
-- logger.dbg("no cmd output yet, will check again soon")
end
end

But we'll wait contiuously if it never terminates for some reason... (unlike Trapper:dismissableRunInSubprocess() where we send a kill before collecting it).

I can somewhat reliably break subsequent dictionary lookups by repeatedly cancelling early on a PW2.

With this PR or without?
I've never seen that on my Kobo, so no idea why - and if sdcv allows concurent executions.

@Frenzie

This comment has been minimized.

Copy link
Member

commented Aug 3, 2019

On that note, by break reliably do you mean temporarily (meaning it'll work again after a little while) or "forever"?

@NiLuJe

This comment has been minimized.

Copy link
Member

commented Aug 3, 2019

That was before this PR, yeah, but with the Trapperized variant (i.e., fuzzy match).

And it appeared to be "forever" (i.e., had to restart KOReader).

To be clear, when it broke, everything instantly returned an empty (no match) result.

Never saw that happen on a Kobo, but, then, I rarely have cause to cancel a lookup, ever.

So that was an extremely edge-casey behavior when testing the original report from MR: essentially doing a quick double-tap ASAP after the hold.
And possibly quickly doing that a couple times until it went kaput ;).

@Frenzie Frenzie merged commit a82d783 into koreader:master Aug 3, 2019

1 check passed

ci/circleci Your tests passed on CircleCI!
Details

@poire-z poire-z deleted the poire-z:dict_always_interruptible branch Aug 3, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.