-
Notifications
You must be signed in to change notification settings - Fork 53
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
abstract: make fibers cancellable #224
abstract: make fibers cancellable #224
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi! Thank you for the patch.
See several comments bellow.
@@ -169,7 +169,7 @@ local function utubettl_fiber(self) | |||
if box.info.ro == false then | |||
local stat, err = pcall(utubettl_fiber_iteration, self, processed) | |||
|
|||
if not stat and not err.code == box.error.READONLY then | |||
if not stat and not (err.code == box.error.READONLY) then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please move this to a separate commit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
queue/abstract/queue_state.lua
Outdated
current = queue_state.states.RUNNING | ||
log.info('Queue state changed: RUNNING') | ||
end | ||
box.ctl.wait_rw() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I understand correctly, we can keep calling with pcall
and in case of an error write a message to the log, check if fiber
is canceled and then throw a new error with more additional context. If this is the case, I prefer to do the extra processing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed to:
--- a/queue/abstract/queue_state.lua
+++ b/queue/abstract/queue_state.lua
@@ -85,8 +85,12 @@ local function create_state_fiber(on_state_change_cb)
log.info('Queue state changed: WAITING')
end
end
+ -- Handle server shutdown.
+ if not pcall(fiber.testcancel) then break end
end
end)
+
+ log.info('Finished queue state fiber')
end
-- Public methods.
99fed57
to
38852a5
Compare
We are going to finish all client (non-system) fibers in the process of Tarantool shutdown. First we cancel them and then wait for their finishing. So if the client fibers are not finished Tarantool shutdown is never finished too. Currently the fiber can not be finished and we got hang in integration testing of PR tarantool/tarantool#9604.
Same as fixing a typo in e355387 ("fix fifottl_fiber_iteration error handling") for fifottl. Required to pass integration testing of PR tarantool/tarantool#9604.
38852a5
to
47378d2
Compare
We are going to finish all client (non-system) fibers in the process of Tarantool shutdown. First we cancel them and then wait for their finishing. So if the client fibers are not finished Tarantool shutdown is never finished too.
Currently some of queue fibers can not be finished and we got hang on integration testing of PR [1]. Let's fix it.
[1] PR with client fibers finishing on shutdown.
tarantool/tarantool#9604