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

Add MuxError handling in FoldBlocksError #548

Merged
merged 1 commit into from
Jun 10, 2024

Conversation

carbolymer
Copy link
Contributor

@carbolymer carbolymer commented Jun 6, 2024

Changelog

- description: |
    Add MuxError handling in `FoldBlocksError`. Rename `LedgerStateCondition` to `ConditionResult`.
# uncomment types applicable to the change:
  type:
  # - feature        # introduces a new feature
   - breaking       # the API has changed in a breaking way
  # - compatible     # the API has changed but is non-breaking
  # - optimisation   # measurable performance improvements
   - refactoring    # QoL changes
  # - bugfix         # fixes a defect
  # - test           # fixes/modifies tests
  # - maintenance    # not directly related to the code
  # - release        # related to a new release preparation
  # - documentation  # change in code docs, haddocks...

Context

Moves handling of errors removed in

to cardano-api. Seems that linking of epoch state logging thread sometimes results in errors:

MuxError MuxBearerClosed "<socket: 71> closed when reading data, waiting on next header True"

or

Exception: ExceptionInLinkedThread (ThreadId 54) (MuxError (MuxIOException writev: resource vanished (Broken pipe)) "(sendAll errored)")

e.g. https://github.com/IntersectMBO/cardano-node/pull/5869/checks?check_run_id=25887156950. This change should let that error be captured by FoldBlocksError.

Also follow-up from

to make LedgerStateCondition more reusable.

Checklist

  • Commit sequence broadly makes sense and commits have useful messages
  • New tests are added if needed and existing tests are updated. See Running tests for more details
  • Self-reviewed the diff

, LedgerStateCondition(..)
, ConditionResult(..)
, fromConditionResult
, toConditionResult
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not super satisfied with those names. Any suggestions are welcome here.

Copy link
Contributor

Choose a reason for hiding this comment

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

For fromConditionResult you could call it: isConditionMet or wasConditionMet. For toConditionResult, I am not sure what could be better, one idea would be simply conditionResult, because it is kind of a smart constructor. Or conditionResultMet?

Copy link
Contributor

Choose a reason for hiding this comment

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

But it doesn't look like it is being used

Copy link
Contributor

Choose a reason for hiding this comment

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

Those names are fine to me. Explicit and not too long. I'd keep them 👍

Copy link
Contributor Author

@carbolymer carbolymer Jun 7, 2024

Choose a reason for hiding this comment

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

They're meant to be used in cardano-testnet if needed. Example usage:

fooCallbck = do
   ...
   pure $ toConditionResult isSkyBlue

or

fooCallbck = do
   ...
   pure $ conditionResult isSkyBlue

Not sure which one is more obvious. I think I'll stick to toConditionResult as it signals simple conversion.

Copy link
Contributor

@palas palas left a comment

Choose a reason for hiding this comment

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

Looks good 👍

, LedgerStateCondition(..)
, ConditionResult(..)
, fromConditionResult
, toConditionResult
Copy link
Contributor

Choose a reason for hiding this comment

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

For fromConditionResult you could call it: isConditionMet or wasConditionMet. For toConditionResult, I am not sure what could be better, one idea would be simply conditionResult, because it is kind of a smart constructor. Or conditionResultMet?

, LedgerStateCondition(..)
, ConditionResult(..)
, fromConditionResult
, toConditionResult
Copy link
Contributor

Choose a reason for hiding this comment

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

But it doesn't look like it is being used

Copy link
Contributor

@smelc smelc left a comment

Choose a reason for hiding this comment

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

LGTM 👍

, LedgerStateCondition(..)
, ConditionResult(..)
, fromConditionResult
, toConditionResult
Copy link
Contributor

Choose a reason for hiding this comment

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

Those names are fine to me. Explicit and not too long. I'd keep them 👍

@carbolymer carbolymer force-pushed the mgalazyn/chore/add-mux-error-foldblocks-error branch from b324de5 to c615f8a Compare June 7, 2024 10:02
@carbolymer carbolymer enabled auto-merge June 7, 2024 10:04
@@ -356,13 +359,15 @@ data FoldBlocksError
= FoldBlocksInitialLedgerStateError !InitialLedgerStateError
Copy link
Contributor

Choose a reason for hiding this comment

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

We should also change the name of this type to something like NewEpochStateError (not necessary for this PR).

@carbolymer carbolymer force-pushed the mgalazyn/chore/add-mux-error-foldblocks-error branch from c615f8a to 6a2f5df Compare June 7, 2024 14:16
@smelc smelc disabled auto-merge June 10, 2024 08:34
@smelc smelc merged commit 4fb7d59 into main Jun 10, 2024
22 of 32 checks passed
@smelc smelc deleted the mgalazyn/chore/add-mux-error-foldblocks-error branch June 10, 2024 08:34
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.

4 participants