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.

Sign up for GitHub

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 IPersistencePlugin inconsistency #2576

Closed
wants to merge 15 commits into from
Closed

fix IPersistencePlugin inconsistency #2576

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
7d344cf
fix IPersistencePlugin inconsistency
Aug 17, 2021
a2c713b
optimize
Aug 17, 2021
ed5d680
Merge branch 'master' into fix_IPersistencePlugin_inconsistency
Qiao-Jin Aug 26, 2021
d19fa73
Merge branch 'master' into fix_IPersistencePlugin_inconsistency
shargon Sep 2, 2021
6023f76
Merge branch 'master' into fix_IPersistencePlugin_inconsistency
Qiao-Jin Sep 13, 2021
ecc664a
Merge branch 'master' into fix_IPersistencePlugin_inconsistency
Qiao-Jin Sep 18, 2021
46cc3b9
Merge branch 'master' into fix_IPersistencePlugin_inconsistency
Qiao-Jin Sep 28, 2021
82cd3c9
Merge branch 'master' into fix_IPersistencePlugin_inconsistency
shargon Sep 28, 2021
d996d54
Merge branch 'master' into fix_IPersistencePlugin_inconsistency
Qiao-Jin Nov 3, 2021
cb5d7ae
Merge branch 'master' into fix_IPersistencePlugin_inconsistency
Qiao-Jin Nov 4, 2021
31cbb82
Merge branch 'master' into fix_IPersistencePlugin_inconsistency
Qiao-Jin Nov 10, 2021
0603d94
Merge branch 'master' into fix_IPersistencePlugin_inconsistency
Qiao-Jin Nov 10, 2021
6d54ef8
Merge branch 'master' into fix_IPersistencePlugin_inconsistency
Qiao-Jin Nov 23, 2021
ef5ce28
Merge branch 'master' into fix_IPersistencePlugin_inconsistency
Qiao-Jin Nov 24, 2021
0207615
Merge branch 'master' into fix_IPersistencePlugin_inconsistency
Qiao-Jin Dec 17, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 19 additions & 11 deletions src/neo/Ledger/Blockchain.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -415,28 +415,36 @@ private void Persist(Block block)
Context.System.EventStream.Publish(application_executed);
all_application_executed.Add(application_executed);
}
foreach (IPersistencePlugin plugin in Plugin.PersistencePlugins)
plugin.OnPersist(system, block, snapshot, all_application_executed);
snapshot.Commit();
List<Exception> commitExceptions = null;
foreach (IPersistencePlugin plugin in Plugin.PersistencePlugins)
{
try
if (plugin.CurrentHeight < block.Index)
{
plugin.OnCommit(system, block, snapshot);
plugin.OnPersist(system, block, snapshot, all_application_executed);
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe OnPersist and OnCommit could be merged then? Previously they were done at different stages of processing, but now they're very similar and probably this separation is not really needed. It will change plugin interface though (but I doubt we have a lot of third-party plugins now).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is introduced in #568:

If there are multiple persistence plugins loaded and one of them throws an exception, then none of them should actually commit anything to a persistent store. To facilitate this behavior, I added an OnCommit method to be called after all plugins have had a chance to process results from OnPersist and throw if something was wrong.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, we should merge 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.

I think we'd better keep this strategy to make sure there is no dirty data stored in plugins once some error occurs

Copy link
Member

Choose a reason for hiding this comment

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

But it's better to revert if there is an error

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Once we merge these 2 functions, it will not be able to work correctly for such scenarios: i.e. there are 2 IPersistencePlugins installed. The first one persists without error but the second one throws some exception caused by dirty data. We can restart nodes again to re-persist data for neo-node and IPersistencePlugins, but the first plugin is already "polluted", I'm afraid the node can only be re-syncronized from 0 height again...

Copy link
Member

Choose a reason for hiding this comment

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

If there are two plugins, and one throw an exception the problem will be in the plugin, I still thinking that is better to merge it. What do you think @erikzhang ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ping

Copy link
Contributor Author

@Qiao-Jin Qiao-Jin Nov 9, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

@erikzhang Could you provide opinion upon this question?

}
catch (Exception ex)
}
List<Exception> commitExceptions = null;
foreach (IPersistencePlugin plugin in Plugin.PersistencePlugins)
{
if (plugin.CurrentHeight < block.Index)
{
if (plugin.ShouldThrowExceptionFromCommit(ex))
try
{
if (commitExceptions == null)
commitExceptions = new List<Exception>();
plugin.OnCommit(system, block, snapshot);
}
catch (Exception ex)
{
if (plugin.ShouldThrowExceptionFromCommit(ex))
{
if (commitExceptions == null)
commitExceptions = new List<Exception>();

commitExceptions.Add(ex);
commitExceptions.Add(ex);
}
}
}
}
if (commitExceptions != null) throw new AggregateException(commitExceptions);
snapshot.Commit();
Copy link
Contributor

Choose a reason for hiding this comment

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

If snapshot is the last to commit then #2570 (comment) is probably not relevant though, plugins won't ever have N-1 data, rather they could have some N+1, but they won't process it again because of plugin.CurrentHeight. So it should be fine.

system.MemPool.UpdatePoolForBlockPersisted(block, snapshot);
}
extensibleWitnessWhiteList = null;
Expand Down
5 changes: 5 additions & 0 deletions src/neo/Plugins/IPersistencePlugin.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,11 @@ namespace Neo.Plugins
/// </summary>
public interface IPersistencePlugin
{
/// <summary>
/// Indicate current persisted height.
/// </summary>
uint CurrentHeight { get; }

/// <summary>
/// Called when a block is being persisted.
/// </summary>
Expand Down
6 changes: 5 additions & 1 deletion tests/neo.UnitTests/Plugins/UT_Plugin.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,10 @@ private class DummyP2PPlugin : IP2PPlugin
{
public void OnVerifiedInventory(IInventory inventory) { }
}
private class DummyPersistencePlugin : IPersistencePlugin { }
private class DummyPersistencePlugin : IPersistencePlugin
{
public uint CurrentHeight => 0;
}

[TestMethod]
public void TestIP2PPlugin()
Expand All @@ -35,6 +38,7 @@ public void TestIPersistencePlugin()

pp.OnCommit(null, null, null);
pp.OnPersist(null, null, null, null);
Assert.AreEqual(pp.CurrentHeight, 0u);
}

[TestMethod]
Expand Down