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

fix(queues): continue processing after hard error in handler #11423

Merged
merged 9 commits into from
Aug 23, 2023

Conversation

sabertobihwy
Copy link
Contributor

@sabertobihwy sabertobihwy commented Aug 18, 2023

Summary

There was an issue with the queue implementation where processing would stop when encountering a hard error in the handler function. The changes in the code fix this problem.

Now, the handler function is called using pcall to catch any errors that occur. If a hard error occurs, the error is logged but the processing of the remaining entries in the queue continues.

Checklist

Full changelog

  • [Implement ...]

Issue reference

Fix KAG-2361

@sabertobihwy sabertobihwy marked this pull request as ready for review August 18, 2023 08:34
if not status then
-- We just log the error and continue, since we want to
-- continue executing the remaining entries in the queue.
self:log_err("handler processed %d entries failed, err: %s", entry_count, ok)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of simply continuing as if the call succeeded, we should treat the error like one that was returned by the handler. This will cause the call to be retried according to the retry policy, which is preferable in my opinion.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updates

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is no need for a nested conditional. If the ok is true, then log the debug message and break. Otherwise, log a message that indicates whether the handler returned a failure or it failed executing altogether (with the respective error message). Also remove the comment "We just log the error and continue, since we want to continue executing the remaining entries in the queue." because it no longer reflects what the code does.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

update

kong/tools/queue.lua Outdated Show resolved Hide resolved
if not status then
-- We just log the error and continue, since we want to
-- continue executing the remaining entries in the queue.
self:log_err("handler processed %d entries failed, err: %s", entry_count, ok)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is no need for a nested conditional. If the ok is true, then log the debug message and break. Otherwise, log a message that indicates whether the handler returned a failure or it failed executing altogether (with the respective error message). Also remove the comment "We just log the error and continue, since we want to continue executing the remaining entries in the queue." because it no longer reflects what the code does.

kong/tools/queue.lua Outdated Show resolved Hide resolved
Copy link
Contributor

@hanshuebner hanshuebner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would have committed the changes myself, but as you're not working off the Kong repository but instead use your own fork, I can't easily do that. Replace everything between if status and ok == true then and self:log_warn("handler could not... with:

    if status and ok then
      self:log_debug("handler processed %d entries successfully", entry_count)
      break
    end
    if not status then
      -- protected call failed, ok is the error message
      err = ok
    end

    self:log_warn("handler could not process entries: %s", tostring(err or "no error details returned by handler"))

    if (now() - start_time) > self.max_retry_time then
      self:log_err(
        "could not send entries, giving up after %d retries.  %d queue entries were lost",
        retry_count, entry_count)
      break
    end

@windmgc windmgc merged commit ee2a9c9 into Kong:master Aug 23, 2023
21 checks passed
@outsinre
Copy link
Contributor

Is this in EE?

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

Successfully merging this pull request may close these issues.

4 participants