fix(healthcheck) retry locking protected fn #37
Conversation
lib/resty/healthcheck.lua
Outdated
|
|
||
|
|
||
| -- Run the given function holding a lock on the target. | ||
| -- WARNING: the callback will run unprotected, so it should never |
There was a problem hiding this comment.
I don't understand why this comment is needed. What's the advantage of invoking the callback in an unprotected way? I'd rather use pcall and remove this
There was a problem hiding this comment.
Good call! Fixed, thanks.
|
@locao I believe |
When ngx.sleep API is not available (e.g. in the log phase) it's not possible to lock using lua-resty-lock and functions that must run protected were failing. This change adds retry methods that start new light threads that have access to ngx.sleep and will succeed to lock. Fix Kong/kong#5137
locao
force-pushed
the
fix/lock_in_log_phase
branch
from
0ce06a2 to
8c6a96a
Compare
There was a problem hiding this comment.
I don't think the test case there actually tests for the condition (we'd need multiple workers hitting that report_http_status call at the same time) so even if we could do it, a failure report would result in a flaky test at best, unless we could mock something that would force a delay within the locked region and then attempt a second request.
For what is worth, I ran the Kong balancer tests on my machine with this branch, but I don't think they exercise concurrency that much either.
|
The timer delays adding targets, so it will get an error if use the get_target_status function to get the status of a node before the targets assigned. |
|
the locking_target_list delays the initialization and update of the targets. |
|
@dabue please do not respond to closed issues/PR's please open a new one if needed |
When ngx.sleep API is not available (e.g. in the log phase) it's not possible to lock using lua-resty-lock and functions that must run protected were failing.
This change adds a retry method that starts a new light thread that has access to ngx.sleep and will succeed to lock.
Fix Kong/kong#5137