From 8f4a6ff2f07c406af5ae9629126c7e8bd87f6746 Mon Sep 17 00:00:00 2001 From: Dmitry Timofeev Date: Fri, 27 Sep 2019 22:10:33 +0300 Subject: [PATCH 1/3] Add ServiceRuntime#beforeCommit [ECR-3584]. --- .../binding/core/runtime/ServiceRuntime.java | 26 ++++++++ .../core/runtime/ServiceRuntimeAdapter.java | 17 ++++++ .../binding/core/runtime/ServiceWrapper.java | 4 ++ .../exonum/binding/core/service/Service.java | 15 +++++ .../binding/core/storage/database/Fork.java | 13 +++- .../runtime/ServiceRuntimeAdapterTest.java | 13 ++++ .../ServiceRuntimeIntegrationTest.java | 61 +++++++++++++++++++ 7 files changed, 147 insertions(+), 2 deletions(-) diff --git a/exonum-java-binding/core/src/main/java/com/exonum/binding/core/runtime/ServiceRuntime.java b/exonum-java-binding/core/src/main/java/com/exonum/binding/core/runtime/ServiceRuntime.java index 231ab33ac3..3a9f88ba60 100644 --- a/exonum-java-binding/core/src/main/java/com/exonum/binding/core/runtime/ServiceRuntime.java +++ b/exonum-java-binding/core/src/main/java/com/exonum/binding/core/runtime/ServiceRuntime.java @@ -302,6 +302,32 @@ private ServiceStateHashes getServiceStateHashes(ServiceWrapper service, Snapsho .build(); } + /** + * Performs the before commit operation for all services in the runtime. + * + * @param fork a fork allowing the runtime and the service to modify the database state. + * Must allow checkpoints and rollbacks. + */ + public void beforeCommit(Fork fork) { + synchronized (lock) { + try { + for (ServiceWrapper service : services.values()) { + fork.createCheckpoint(); + try { + service.beforeCommit(fork); + } catch (Exception e) { + logger.error("Service {} threw exception in beforeCommit. Any changes are rolled-back", + service.getName(), e); + fork.rollback(); + } + } + } catch (Exception e) { + logger.error("Unexpected exception in beforeCommit", e); + throw e; + } + } + } + /** * Notifies the services in the runtime of the block commit event. */ diff --git a/exonum-java-binding/core/src/main/java/com/exonum/binding/core/runtime/ServiceRuntimeAdapter.java b/exonum-java-binding/core/src/main/java/com/exonum/binding/core/runtime/ServiceRuntimeAdapter.java index 96e97b7eb1..aedf36bf91 100644 --- a/exonum-java-binding/core/src/main/java/com/exonum/binding/core/runtime/ServiceRuntimeAdapter.java +++ b/exonum-java-binding/core/src/main/java/com/exonum/binding/core/runtime/ServiceRuntimeAdapter.java @@ -199,6 +199,23 @@ byte[] getStateHashes(long snapshotHandle) throws CloseFailuresException { } } + /** + * Performs the before commit operation for services in this runtime. + * + * @param forkHandle a handle to the native fork object, which must support checkpoints + * and rollbacks + * @throws CloseFailuresException if there was a failure in destroying some native peers + * @see ServiceRuntime#beforeCommit(Fork) + */ + void beforeCommit(long forkHandle) throws CloseFailuresException { + try (Cleaner cleaner = new Cleaner("beforeCommit")) { + Fork fork = viewFactory.createFork(forkHandle, cleaner); + serviceRuntime.beforeCommit(fork); + } catch (CloseFailuresException e) { + handleCloseFailure(e); + } + } + /** * Notifies the runtime of the block commit event. * diff --git a/exonum-java-binding/core/src/main/java/com/exonum/binding/core/runtime/ServiceWrapper.java b/exonum-java-binding/core/src/main/java/com/exonum/binding/core/runtime/ServiceWrapper.java index 799036a656..7787ee8443 100644 --- a/exonum-java-binding/core/src/main/java/com/exonum/binding/core/runtime/ServiceWrapper.java +++ b/exonum-java-binding/core/src/main/java/com/exonum/binding/core/runtime/ServiceWrapper.java @@ -85,6 +85,10 @@ List getStateHashes(Snapshot snapshot) { return service.getStateHashes(snapshot); } + void beforeCommit(Fork fork) { + service.beforeCommit(fork); + } + void afterCommit(BlockCommittedEvent event) { service.afterCommit(event); } diff --git a/exonum-java-binding/core/src/main/java/com/exonum/binding/core/service/Service.java b/exonum-java-binding/core/src/main/java/com/exonum/binding/core/service/Service.java index a9a69dd6f8..5fd2edc512 100644 --- a/exonum-java-binding/core/src/main/java/com/exonum/binding/core/service/Service.java +++ b/exonum-java-binding/core/src/main/java/com/exonum/binding/core/service/Service.java @@ -98,6 +98,21 @@ default void configure(Fork fork, Configuration configuration) { */ void createPublicApiHandlers(Node node, Router router); + /** + * Handles the changes made by all transactions included in the upcoming block. + * This handler is an optional callback method invoked by the blockchain after all transactions + * in a block are executed, but before it is committed. The service can modify its state + * in this handler, therefore, implementations must be deterministic and use only the current + * database state as their input. + * + *

This method is invoked synchronously from the thread that commits the block, therefore, + * implementations of this method must not perform any blocking or long-running operations. + * + *

Any exceptions in this method will revert any changes made to the database by it, + * but will not affect the processing of this block. + */ + default void beforeCommit(Fork fork) {} + /** * Handles read-only block commit event. This handler is an optional callback method which is * invoked by the blockchain after each block commit. For example, a service can create one or diff --git a/exonum-java-binding/core/src/main/java/com/exonum/binding/core/storage/database/Fork.java b/exonum-java-binding/core/src/main/java/com/exonum/binding/core/storage/database/Fork.java index 30316a81c3..3a8125cc87 100644 --- a/exonum-java-binding/core/src/main/java/com/exonum/binding/core/storage/database/Fork.java +++ b/exonum-java-binding/core/src/main/java/com/exonum/binding/core/storage/database/Fork.java @@ -153,8 +153,15 @@ NativeHandle intoPatch() { /** * Creates in-memory checkpoint that can be used to rollback changes. + * + *

Creating a checkpoint will invalidate all collections that were instantiated with this fork. */ - void createCheckpoint() { + public void createCheckpoint() { + // todo: (1) Are nested rollbacks supported, or is the existing checkpoint lost when a new one + // is created? + // (2) If it is, then these methods being public will allow the broken services + // to rollback the changes made by *all the previous services*! + // Shall we prevent that? checkState(nativeCanRollback(getNativeHandle()), "This fork does not support checkpoints"); @@ -168,8 +175,10 @@ void createCheckpoint() { * this particular Fork instance. * *

If no checkpoints was created, rollbacks all changes made by this fork. + * + *

Rollback will invalidate all collections that were created with this fork. */ - void rollback() { + public void rollback() { checkState(nativeCanRollback(getNativeHandle()), "This fork does not support rollbacks"); diff --git a/exonum-java-binding/core/src/test/java/com/exonum/binding/core/runtime/ServiceRuntimeAdapterTest.java b/exonum-java-binding/core/src/test/java/com/exonum/binding/core/runtime/ServiceRuntimeAdapterTest.java index 99ae804d04..6545283f4a 100644 --- a/exonum-java-binding/core/src/test/java/com/exonum/binding/core/runtime/ServiceRuntimeAdapterTest.java +++ b/exonum-java-binding/core/src/test/java/com/exonum/binding/core/runtime/ServiceRuntimeAdapterTest.java @@ -22,6 +22,7 @@ import static org.junit.jupiter.api.Assertions.assertThrows; import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.eq; +import static org.mockito.Mockito.mock; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; @@ -138,6 +139,18 @@ void configureServiceNotAny() { assertThat(e).hasMessageContainingAll("Any", toHexString(invalidConfig)); } + @Test + void beforeCommit() throws CloseFailuresException { + long forkHandle = 0x110b; + Fork fork = mock(Fork.class); + when(viewFactory.createFork(eq(forkHandle), any(Cleaner.class))) + .thenReturn(fork); + + serviceRuntimeAdapter.beforeCommit(forkHandle); + + verify(serviceRuntime).beforeCommit(fork); + } + @Test void afterCommit_ValidatorNode() throws CloseFailuresException { when(viewFactory.createSnapshot(eq(SNAPSHOT_HANDLE), any(Cleaner.class))) diff --git a/exonum-java-binding/core/src/test/java/com/exonum/binding/core/runtime/ServiceRuntimeIntegrationTest.java b/exonum-java-binding/core/src/test/java/com/exonum/binding/core/runtime/ServiceRuntimeIntegrationTest.java index 1c30476c58..2bc80e0710 100644 --- a/exonum-java-binding/core/src/test/java/com/exonum/binding/core/runtime/ServiceRuntimeIntegrationTest.java +++ b/exonum-java-binding/core/src/test/java/com/exonum/binding/core/runtime/ServiceRuntimeIntegrationTest.java @@ -30,6 +30,7 @@ import static org.mockito.ArgumentMatchers.any; import static org.mockito.Mockito.doThrow; import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.spy; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; @@ -49,6 +50,7 @@ import com.exonum.binding.core.transaction.TransactionContext; import com.exonum.binding.core.transport.Server; import com.google.common.collect.ImmutableMap; +import com.google.common.collect.Lists; import com.google.protobuf.Any; import com.google.protobuf.ByteString; import io.vertx.ext.web.Router; @@ -367,6 +369,34 @@ void getStateHashesSingleService() throws CloseFailuresException { } } + @Test + void beforeCommitSingleService() throws CloseFailuresException { + try (Database database = TemporaryDb.newInstance(); + Cleaner cleaner = new Cleaner()) { + Fork fork = database.createFork(cleaner); + + serviceRuntime.beforeCommit(fork); + + verify(serviceWrapper).beforeCommit(fork); + } + } + + @Test + void beforeCommitThrowingServiceChangesAreRolledBack() throws CloseFailuresException { + try (Database database = TemporaryDb.newInstance(); + Cleaner cleaner = new Cleaner()) { + Fork fork = spy(database.createFork(cleaner)); + doThrow(IllegalStateException.class).when(serviceWrapper).beforeCommit(fork); + + serviceRuntime.beforeCommit(fork); + + InOrder inOrder = Mockito.inOrder(fork, serviceWrapper); + inOrder.verify(fork).createCheckpoint(); + inOrder.verify(serviceWrapper).beforeCommit(fork); + inOrder.verify(fork).rollback(); + } + } + @Test void afterCommitSingleService() { BlockCommittedEvent event = mock(BlockCommittedEvent.class); @@ -479,6 +509,37 @@ void getStateHashesMultipleServices() throws CloseFailuresException { } } + @Test + void beforeCommitMultipleServicesWithFirstThrowing() throws CloseFailuresException { + try (Database database = TemporaryDb.newInstance(); + Cleaner cleaner = new Cleaner()) { + Collection services = SERVICES.values(); + + // Setup the first service to throw exception in its before commit handler + ServiceWrapper service1 = services + .iterator() + .next(); + Fork fork = spy(database.createFork(cleaner)); + doThrow(RuntimeException.class).when(service1).beforeCommit(fork); + + // Notify the runtime before the block commit + serviceRuntime.beforeCommit(fork); + + // Verify that each service got the notifications, i.e., the first service + // throwing an exception has not disrupted the notification process + Object[] mocks = Lists.asList(fork, services.toArray()).toArray(); + InOrder inOrder = Mockito.inOrder(mocks); + for (ServiceWrapper service : services) { + inOrder.verify(fork).createCheckpoint(); + inOrder.verify(service).beforeCommit(fork); + // Verify the fork was rolled-back once after the throwing service + if (service.equals(service1)) { + inOrder.verify(fork).rollback(); + } + } + } + } + @Test void afterCommitMultipleServicesWithFirstThrowing() { Collection services = SERVICES.values(); From df7cb849668e410559d220e2dc65046f94b07fd7 Mon Sep 17 00:00:00 2001 From: Dmitry Timofeev Date: Fri, 27 Sep 2019 22:12:50 +0300 Subject: [PATCH 2/3] Fix deprecation warning: Replace the mistakenly imported assertThat method with the main one. --- .../storage/database/NativeResourceManagerIntegrationTest.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/exonum-java-binding/core/src/test/java/com/exonum/binding/core/storage/database/NativeResourceManagerIntegrationTest.java b/exonum-java-binding/core/src/test/java/com/exonum/binding/core/storage/database/NativeResourceManagerIntegrationTest.java index 403811a3f5..b560d54db3 100644 --- a/exonum-java-binding/core/src/test/java/com/exonum/binding/core/storage/database/NativeResourceManagerIntegrationTest.java +++ b/exonum-java-binding/core/src/test/java/com/exonum/binding/core/storage/database/NativeResourceManagerIntegrationTest.java @@ -16,7 +16,7 @@ package com.exonum.binding.core.storage.database; -import static org.assertj.core.api.Java6Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThat; import static org.junit.jupiter.api.Assertions.assertThrows; import com.exonum.binding.common.serialization.StandardSerializers; From 4aee8d1dda211327e83474699bc89b0fb02f8955 Mon Sep 17 00:00:00 2001 From: Dmitry Timofeev Date: Thu, 3 Oct 2019 13:35:28 +0300 Subject: [PATCH 3/3] Add the test when multiple checkpoints are created: Also, resolve the todo and improve the documentation. --- .../binding/core/storage/database/Fork.java | 43 +++++++++------- .../storage/database/ForkIntegrationTest.java | 51 +++++++++++++++++++ 2 files changed, 75 insertions(+), 19 deletions(-) diff --git a/exonum-java-binding/core/src/main/java/com/exonum/binding/core/storage/database/Fork.java b/exonum-java-binding/core/src/main/java/com/exonum/binding/core/storage/database/Fork.java index 3a8125cc87..c5516f1cdb 100644 --- a/exonum-java-binding/core/src/main/java/com/exonum/binding/core/storage/database/Fork.java +++ b/exonum-java-binding/core/src/main/java/com/exonum/binding/core/storage/database/Fork.java @@ -152,16 +152,20 @@ NativeHandle intoPatch() { } /** - * Creates in-memory checkpoint that can be used to rollback changes. + * Creates in-memory checkpoint of the current state of this Fork. A checkpoint allows to restore + * the state of the Fork by reverting the changes made since the last checkpoint operation with + * {@link #rollback()}. The changes made before the last checkpoint cannot be reverted, + * because each new checkpoint replaces the previous: checkpoints are not stacked. * *

Creating a checkpoint will invalidate all collections that were instantiated with this fork. + * + *

This operation is not intended to be used by services. */ public void createCheckpoint() { - // todo: (1) Are nested rollbacks supported, or is the existing checkpoint lost when a new one - // is created? - // (2) If it is, then these methods being public will allow the broken services - // to rollback the changes made by *all the previous services*! - // Shall we prevent that? + // As stacked (nested) checkpoints are not supported, this operation must not be used by + // the client code, because in case of an exception it will make the framework + // unable to revert the changes made by the service before the service created + // a checkpoint: ECR-3611 checkState(nativeCanRollback(getNativeHandle()), "This fork does not support checkpoints"); @@ -171,12 +175,13 @@ public void createCheckpoint() { } /** - * Rollbacks changes to the latest checkpoint. Affects only changes made with - * this particular Fork instance. - * - *

If no checkpoints was created, rollbacks all changes made by this fork. + * Rollbacks changes to the latest checkpoint. If no checkpoints were created, rollbacks all + * changes made by this fork. Rollback affects only changes made with this particular + * Fork instance. * *

Rollback will invalidate all collections that were created with this fork. + * + *

This operation is not intended to be used by services. */ public void rollback() { checkState(nativeCanRollback(getNativeHandle()), @@ -230,6 +235,15 @@ private void replaceIndexCleaner() { */ private static native long nativeIntoPatch(long nativeHandle); + /** + * Returns true if creating checkpoints and performing rollbacks is + * possible with this particular Fork instance. + * + * @see #createCheckpoint() + * @see #rollback() + */ + private static native boolean nativeCanRollback(long nativeHandle); + /** * Creates in-memory checkpoint that can be used to rollback changes. */ @@ -240,13 +254,4 @@ private void replaceIndexCleaner() { * this particular Fork instance. */ private static native void nativeRollback(long nativeHandle); - - /** - * Returns true if creating checkpoints and performing rollbacks is - * possible with this particular Fork instance. - * - * @see #createCheckpoint() - * @see #rollback() - */ - private static native boolean nativeCanRollback(long nativeHandle); } diff --git a/exonum-java-binding/core/src/test/java/com/exonum/binding/core/storage/database/ForkIntegrationTest.java b/exonum-java-binding/core/src/test/java/com/exonum/binding/core/storage/database/ForkIntegrationTest.java index 66db70c1d0..0d0a905fae 100644 --- a/exonum-java-binding/core/src/test/java/com/exonum/binding/core/storage/database/ForkIntegrationTest.java +++ b/exonum-java-binding/core/src/test/java/com/exonum/binding/core/storage/database/ForkIntegrationTest.java @@ -29,6 +29,7 @@ import com.exonum.binding.core.storage.indices.ListIndexProxy; import com.exonum.binding.test.RequiresNativeLibrary; import java.util.Iterator; +import org.junit.jupiter.api.DisplayName; import org.junit.jupiter.api.Test; @RequiresNativeLibrary @@ -147,6 +148,56 @@ void rollbacksChangesMadeSinceLastCheckpoint() throws Exception { } } + @Test + @DisplayName("rollback shall work when multiple checkpoints are created, reverting to the" + + "state as of the last checkpoint") + void rollbacksToTheLastCheckpointWhenMultipleAreCreated() throws Exception { + try (TemporaryDb db = TemporaryDb.newInstance(); + Cleaner cleaner = new Cleaner("parent")) { + Fork fork = db.createFork(cleaner); + + // Create a list with a single element + String listName = "list"; + + // Modify and create the first checkpoint + { + ListIndex list = newList(listName, fork); + list.add("s1"); + fork.createCheckpoint(); + } + + // Modify and create the second checkpoint + { + ListIndex list = newList(listName, fork); + list.add("s2"); + fork.createCheckpoint(); + } + + // Modify the list + { + ListIndex list = newList(listName, fork); + list.add("s3"); + assertThat(list).containsExactly("s1", "s2", "s3"); + } + + // Rollback the changes: must restore the state as of the second checkpoint + fork.rollback(); + { + ListIndex list = newList(listName, fork); + assertThat(list).containsExactly("s1", "s2"); + } + + // Rollback again: as no nested (stacked) checkpoints are supported, + // the first checkpoint is no longer available and any rollback will revert + // the state to the last (second) checkpoint + fork.rollback(); + { + ListIndex list = newList(listName, fork); + assertThat(list).containsExactly("s1", "s2"); + } + } + } + @Test void rollbackDoesNotAffectDatabase() throws Exception { try (TemporaryDb db = TemporaryDb.newInstance();