Skip to content

fix: Deadlock during cost calculation#173

Merged
swithek merged 8 commits intojellydator:v3from
dadrus:fix/cost_calc_deadlock
Jun 5, 2025
Merged

fix: Deadlock during cost calculation#173
swithek merged 8 commits intojellydator:v3from
dadrus:fix/cost_calc_deadlock

Conversation

@dadrus
Copy link
Copy Markdown
Contributor

@dadrus dadrus commented May 16, 2025

Unfortunately, the implementation, I've done in #152 can cause a deadlock during cost calculation:

The update method of the Item locks an RW lock and calls the item.calculateCost(item), which if configured, may make use of the available getter functions of the item object. All these functions make however use of the R lock - thus we have a deadlock.

This PR updates the implementation of the Item and performs the calculation of the item cost on a snapshot rather the actual update.

The test has been updated as well to use a public method. Without the update to the Item implementation the corresponding test will run into a deadlock

@coveralls
Copy link
Copy Markdown

coveralls commented May 16, 2025

Pull Request Test Coverage Report for Build 15467277810

Details

  • 15 of 15 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.001%) to 99.733%

Totals Coverage Status
Change from base Build 15260220298: 0.001%
Covered Lines: 747
Relevant Lines: 749

💛 - Coveralls

@dadrus
Copy link
Copy Markdown
Contributor Author

dadrus commented May 16, 2025

@swithek: Would you please review?

@swithek
Copy link
Copy Markdown
Contributor

swithek commented May 21, 2025

@dadrus thanks for the PR. I'll try to review it by the end of this week.

Comment thread item_test.go Outdated
@dadrus dadrus requested review from davseby and swithek June 3, 2025 08:27
Copy link
Copy Markdown
Contributor

@swithek swithek 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, one small change is needed. Also, you might need to merge/rebase your branch on the latest v3 branch, because there have been some changes there that might affect some of your code (at least the tests).

Comment thread README.md Outdated
@dadrus dadrus requested a review from swithek June 4, 2025 16:51
Copy link
Copy Markdown
Contributor

@davseby davseby left a comment

Choose a reason for hiding this comment

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

Seems good, waiting for @swithek.

Comment thread README.md Outdated
@dadrus dadrus requested a review from davseby June 5, 2025 12:41
@swithek swithek merged commit b20a7fd into jellydator:v3 Jun 5, 2025
2 checks passed
@swithek
Copy link
Copy Markdown
Contributor

swithek commented Jun 5, 2025

Thanks for the change @dadrus Quality work as always 👍

@dadrus
Copy link
Copy Markdown
Contributor Author

dadrus commented Jun 5, 2025

Thank you @swithek and @davseby !

Just a side question: Do you have any specific time frame when you plan to release the next version?

@dadrus dadrus deleted the fix/cost_calc_deadlock branch June 5, 2025 18:07
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