Skip to content

Fix mutex poisoning - #77

Merged
jeffcharles merged 2 commits into
mainfrom
jc.fix-mutex-poisoning
Nov 2, 2026
Merged

jeffcharles merged 2 commits into
mainfrom
jc.fix-mutex-poisoning

Conversation

@jeffcharles

Copy link
Copy Markdown
Contributor

This got flagged as an unrelated issue by a code review agent when I was trying to change something else.

@jeffcharles
jeffcharles requested a review from a team as a code owner October 30, 2026 20:56
@jeffcharles
jeffcharles requested review from adampetro and pauloroberto-pozzato and removed request for a team October 30, 2026 20:56
@jeffcharles
jeffcharles force-pushed the jc.fix-mutex-poisoning branch from 2f11719 to 600a38b Compare October 30, 2026 21:22
@jeffcharles
jeffcharles force-pushed the jc.fix-mutex-poisoning branch from 7048a6d to c0c7ac4 Compare November 2, 2026 15:50

@saga-dasgupta saga-dasgupta left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

One small comment.

Comment thread src/lib.rs

let fn_return = f(self.store);
store_setup_started = true;
setup_store(self.store, self.weight_unit);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Should these 2 lines be swapped in order in case setup store fails?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The store setup potentially failing is why store_setup_started is set first. We want to cleanup our changes to how epoch deadlines are handled if we may have changed how they're handled. I think the idea is to avoid changing the store if we haven't run any setup on it.

That said, with fresh eyes I think there's a better way to handle this.

@jeffcharles jeffcharles Nov 2, 2026 •

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

f9c9b86 moves cleaning up the epoch handling into the Some(ticker) case so we don't need the variable.

@saga-dasgupta saga-dasgupta left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Yes, this is better, thanks.

@jeffcharles
jeffcharles merged commit 3f489e0 into main Nov 2, 2026
5 checks passed
@jeffcharles
jeffcharles deleted the jc.fix-mutex-poisoning branch November 2, 2026 16:12
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