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

Execute Transaction<T>.Actions all or nothing at all #1275

Merged
merged 5 commits into from
May 4, 2021

Conversation

dahlia
Copy link
Contributor

@dahlia dahlia commented Apr 29, 2021

This addresses fixes #1267.

Previously, Transaction<T>.Actions had been executed and affected states whether the actions throw an exception or not. However, as the term “transaction” in computer science in general imply it's atomic (i.e., executed all or nothing), this behavior had been quite misleading.

This patch regards such previous behavior as a bug, and fixes it so that if any action in a Transaction<T> throws an exception all actions in the transaction do not commit any state changes to the current IStore.

However, there is still a quirk for it: IActionRenderer<T> receives render events caused by unsuccessful transactions too, as it has done. In other words, transactions' atomicity still is not guaranteed for action renderers. In order to work around this quirk, this patch introduces AtomicActionRenderer<T> as well. If an IActionRenderer<T> is wrapped by AtomicActionRenderer<T> the inner action renderer will receive no RenderActionError()/UnrenderActionError() events except for block actions (since they do not belong to any transactions).

The reasons why I stick with the existing behavior for action renderers are:

  • Ignoring render events from unsuccessful transactions making the current shape of IActionRenderer<T> no more natural. Because RenderActionError()/UnrenderActionError() methods are no more necessary.
  • However, changing the interface breaks backward compatibility. In particular, IActionRenderer<T> is one of the key interfaces game app programmers must implement. For example, such change on the interface would require many changes on Nine Chronicles.
  • We may eventually need to refactor the interface when we implement Leaving Transaction<T>s' results #1156.

For details, please read the added changelogs and XML doc comments too.

@dahlia dahlia added the bug Something isn't working label Apr 29, 2021
@dahlia dahlia self-assigned this Apr 29, 2021
@codecov
Copy link

codecov bot commented Apr 29, 2021

Codecov Report

Merging #1275 (1474b66) into main (e032b8f) will increase coverage by 0.15%.
The diff coverage is 86.11%.

❗ Current head 1474b66 differs from pull request most recent head e750758. Consider uploading reports for the commit e750758 to get more accurate results

@@            Coverage Diff             @@
##             main    #1275      +/-   ##
==========================================
+ Coverage   83.96%   84.11%   +0.15%     
==========================================
  Files         402      407       +5     
  Lines       34464    34987     +523     
==========================================
+ Hits        28937    29430     +493     
- Misses       3700     3730      +30     
  Partials     1827     1827              
Impacted Files Coverage Δ
Libplanet/Blockchain/BlockChain.cs 90.76% <ø> (+0.83%) ⬆️
...anet/Blockchain/Renderers/DelayedActionRenderer.cs 82.40% <ø> (ø)
Libplanet/Tx/Transaction.cs 90.41% <ø> (ø)
Libplanet.Tests/Fixtures/Arithmetic.cs 44.04% <44.04%> (ø)
Libplanet.Tests/Fixtures/OperatorTypeExtensions.cs 66.66% <66.66%> (ø)
...lanet/Blockchain/Renderers/AtomicActionRenderer.cs 82.95% <82.95%> (ø)
Libplanet.Tests/Fixtures/IntegerSet.cs 92.30% <92.30%> (ø)
Libplanet.Tests/Action/ActionContextTest.cs 94.70% <100.00%> (+1.05%) ⬆️
Libplanet.Tests/Action/ActionEvaluationTest.cs 98.05% <100.00%> (+6.38%) ⬆️
...lockchain/Renderers/AnonymousActionRendererTest.cs 88.39% <100.00%> (ø)
... and 22 more

Copy link
Member

@longfin longfin left a comment

Choose a reason for hiding this comment

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

It seems that need to rebase.

@dahlia dahlia force-pushed the tx-atomicity branch 2 times, most recently from 69b6474 to edeb6ea Compare May 3, 2021 02:05
@longfin longfin requested review from longfin and moreal May 3, 2021 02:32
longfin
longfin previously approved these changes May 3, 2021
Copy link
Contributor

@moreal moreal left a comment

Choose a reason for hiding this comment

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

Thanks to apply reviews 🙏🏻 I believe this is my last review 👍🏻

moreal
moreal previously approved these changes May 3, 2021
Copy link
Contributor

@moreal moreal left a comment

Choose a reason for hiding this comment

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

🚀

@dahlia
Copy link
Contributor Author

dahlia commented May 3, 2021

@planetarium/libplanet PTAL.

riemannulus
riemannulus previously approved these changes May 3, 2021
@dahlia dahlia dismissed stale reviews from riemannulus and moreal via e750758 May 4, 2021 02:16
@dahlia dahlia requested review from riemannulus and moreal May 4, 2021 04:42
@dahlia
Copy link
Contributor Author

dahlia commented May 4, 2021

@planetarium/libplanet All green now. Please take a look again!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Actions in a transaction are executed all or nothing at all
4 participants