Skip to content

Conversation

@qiluo-msft
Copy link
Contributor

Best practice says to not let exceptions propagate out of a destructor. So swallow any exception if it really throws in RedisPipeline class.

@mssonicbld
Copy link
Collaborator

/azp run

@qiluo-msft qiluo-msft marked this pull request as ready for review November 26, 2025 02:18
Copilot AI review requested due to automatic review settings November 26, 2025 02:18
@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR adds exception handling to the RedisPipeline destructor to prevent exceptions from propagating out of the destructor, which is a C++ best practice. The change wraps the existing flush() call in a try-catch block that catches both std::exception and generic exceptions, logging appropriate error messages.

Key changes:

  • Wrapped flush() call in destructor with try-catch blocks to prevent exception propagation
  • Added error logging for both specific exceptions and unknown exceptions during flush

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@mssonicbld
Copy link
Collaborator

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@qiluo-msft qiluo-msft requested a review from r12f November 26, 2025 18:12
@qiluo-msft
Copy link
Contributor Author

qiluo-msft commented Nov 27, 2025

The coverage missing is due to no unit test covering proventative code path. It is easy to cover one line by shutdown redis. Other lines are difficult.

There is challenge to cover them in unit test. It should be safe and no extra harm to merge. It is preventative code path. Originally code will crash in the same situation.

catch (const std::exception& e)
catch (...)

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants