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 misleading 'serious error' message #88

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

Ozaq
Copy link
Contributor

@Ozaq Ozaq commented Feb 19, 2025

During destruction of an AsyncApiIterator an SeriousBug exception is created if the writer task has not yet pushd all output into the shared eckit::Queue. Sending a SeriousBug exception to the writing thread, via eckit::Queue::interrupt, safely stops writing by unwinding the stack until the SeriousBug exception was silently caught in the wrapping lambda.

The issue has been that on construction of a SeriousBug exception error message and stack trace are logged. This implies to the user that a serious defect was encountered when in fact everything is fine.

This change is a bit of a hack as it still uses throwing an exception from the queue on write but now will use an exception that does not log anything.

Fixes: FDB-405

@Ozaq Ozaq added the approved-for-ci Approved for CI run label Feb 19, 2025
@codecov-commenter
Copy link

codecov-commenter commented Feb 19, 2025

Codecov Report

Attention: Patch coverage is 0% with 3 lines in your changes missing coverage. Please review.

Project coverage is 61.49%. Comparing base (baddc85) to head (fa62f76).

Files with missing lines Patch % Lines
src/fdb5/api/helpers/APIIterator.h 0.00% 3 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop      #88      +/-   ##
===========================================
- Coverage    61.50%   61.49%   -0.01%     
===========================================
  Files          275      275              
  Lines        15226    15228       +2     
  Branches      1551     1551              
===========================================
  Hits          9365     9365              
- Misses        5861     5863       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Ozaq Ozaq force-pushed the fix-async-api-iterator-cancellation branch from 9f6f76c to a45da83 Compare February 19, 2025 15:50
Copy link
Member

@ChrisspyB ChrisspyB left a comment

Choose a reason for hiding this comment

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

Does seem a bit hacky... I think I'll defer to Simon for this one.

@Ozaq Ozaq force-pushed the fix-async-api-iterator-cancellation branch from a45da83 to 2d237d7 Compare February 20, 2025 07:18
@Ozaq
Copy link
Contributor Author

Ozaq commented Feb 20, 2025

Does seem a bit hacky... I think I'll defer to Simon for this one.

Indeed. I considered another approach but that's a discussion that we need to have in a different context. Right now this change is just cosmetics.

@Ozaq Ozaq force-pushed the fix-async-api-iterator-cancellation branch from 2d237d7 to 523f00a Compare February 21, 2025 08:00
@Ozaq
Copy link
Contributor Author

Ozaq commented Feb 21, 2025

Addressed comment by @simondsmart to subclass eckit::Exception

During destruction of an AsyncApiIterator an SeriousBug exception is
created if the writer task has not yet pushd all output into the shared
eckit::Queue. Sending a SeriousBug exception to the writing thread, via
eckit::Queue::interrupt, safely stops writing by unwinding the stack
until the SeriousBug exception was silently caught in the wrapping
lambda.

The issue has been that on construction of a SeriousBug exception error
message and stack trace are logged. This implies to the user that a
serious defect was encountered when in fact everything is fine.

This change is a bit of a hack as it still uses throwing an exception
from the queue on write but now will use an exception that does not log
anything.

Fixes: FDB-405
@Ozaq Ozaq force-pushed the fix-async-api-iterator-cancellation branch from 523f00a to fa62f76 Compare February 24, 2025 13:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved-for-ci Approved for CI run
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants