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: race condition on resizing #1742

Merged
merged 1 commit into from
Dec 13, 2023
Merged

Conversation

bonomat
Copy link
Contributor

@bonomat bonomat commented Dec 13, 2023

By setting the position only to closed in our db when we receive the CloseFinalize event we can get rid of the async task closed_positions which checked regularly if a position needs to be closed in the db. This helps us to remove a race condition where a position was accidentally set to closed which was actually in resizing.

Resolves https://github.com/get10101/meta/issues/259
Resolves https://github.com/get10101/meta/issues/262

Replaces #1734

@bonomat bonomat requested review from holzeis and luckysori December 13, 2023 12:50
@bonomat bonomat force-pushed the fix/accidentally-closing-positions branch from bca9266 to 90c8ac3 Compare December 13, 2023 12:51
Copy link
Contributor

@holzeis holzeis left a comment

Choose a reason for hiding this comment

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

LGTM 👍

coordinator/src/node.rs Show resolved Hide resolved
coordinator/src/node.rs Show resolved Hide resolved
// this should never happen because we are only loading `Open` and
// `Resizing` a few lines above
tracing::error!(
channel_id = msg.channel_id.to_hex(),
Copy link
Contributor

Choose a reason for hiding this comment

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

🔧 the comment does not match the match statement.

CHANGELOG.md Outdated
@@ -8,6 +8,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
## [Unreleased]

- Feat: Receive USD-P via Lightning
- Fix: a bug which may lead to a stuck position due to some async tasks
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Instead of writing what it may fix, I would write what has changed. Like replaced a task with explicitly setting the closed state when the protocol finished.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe we need to agree for whom the changelog is 😅
What you describe can be found in the commit message while here I'm writing high-level description only.

Copy link
Contributor

Choose a reason for hiding this comment

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

It would still be high level wouldn't it? 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Mhm, fair enough. I'll extend it and we all will be happy :D

coordinator/src/node/resize.rs Outdated Show resolved Hide resolved
@bonomat bonomat force-pushed the fix/accidentally-closing-positions branch 2 times, most recently from a95d52d to 2cbc7bc Compare December 13, 2023 16:03
By setting the position only to closed in our db when we receive the `CloseFinalize` event we can get rid of the async task `closed_positions` which checked regularly if a position needs to be closed in the db.
This helps us to remove a race condition where a position was accidentally set to closed which was actually in resizing.
@bonomat bonomat force-pushed the fix/accidentally-closing-positions branch from 2cbc7bc to b38a2a2 Compare December 13, 2023 18:08
@bonomat bonomat enabled auto-merge December 13, 2023 18:09
@bonomat bonomat added this pull request to the merge queue Dec 13, 2023
Merged via the queue into main with commit 0a2734d Dec 13, 2023
9 checks passed
@bonomat bonomat deleted the fix/accidentally-closing-positions branch December 13, 2023 18:42
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.

2 participants