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

[Refactor] Don't generate shutdown tasks in controller #141

Merged
merged 3 commits into from
Oct 29, 2019
Merged

[Refactor] Don't generate shutdown tasks in controller #141

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
0709dc9
Remove shutdown actions from ControllerBuilder
mbaxter Oct 28, 2019
2c87f5e
Constrain BesuController's management of closeables
mbaxter Oct 28, 2019
1f5bd20
Merge branch 'master' into refactor/dont-generate-shutdown-tasks-in-c…
mbaxter Oct 29, 2019
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
1 change: 1 addition & 0 deletions besu/src/main/java/org/hyperledger/besu/Runner.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,6 +113,7 @@ public void stop() {
waitForServiceToStop("Mining Coordinator", besuController.getMiningCoordinator()::awaitStop);
if (networkRunner.getNetwork().isP2pEnabled()) {
besuController.getSynchronizer().stop();
waitForServiceToStop("Synchronizer", besuController.getSynchronizer()::awaitStop);
}

networkRunner.stop();
Expand Down
25 changes: 20 additions & 5 deletions besu/src/main/java/org/hyperledger/besu/controller/BesuController.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,11 +31,18 @@
import org.hyperledger.besu.ethereum.mainnet.ProtocolSchedule;
import org.hyperledger.besu.ethereum.p2p.config.SubProtocolConfiguration;

import java.io.Closeable;
import java.io.IOException;
import java.util.Collection;
import java.util.Collections;
import java.util.List;
import java.util.Map;

import org.apache.logging.log4j.LogManager;
import org.apache.logging.log4j.Logger;

public class BesuController<C> implements java.io.Closeable {
private static final Logger LOG = LogManager.getLogger();

public static final String DATABASE_PATH = "database";
private final ProtocolSchedule<C> protocolSchedule;
Expand All @@ -50,7 +57,7 @@ public class BesuController<C> implements java.io.Closeable {
private final TransactionPool transactionPool;
private final MiningCoordinator miningCoordinator;
private final PrivacyParameters privacyParameters;
private final Runnable close;
private final List<Closeable> closeables;
private final SyncState syncState;

BesuController(
Expand All @@ -64,9 +71,9 @@ public class BesuController<C> implements java.io.Closeable {
final TransactionPool transactionPool,
final MiningCoordinator miningCoordinator,
final PrivacyParameters privacyParameters,
final Runnable close,
final JsonRpcMethodFactory additionalJsonRpcMethodsFactory,
final KeyPair keyPair) {
final KeyPair keyPair,
final List<Closeable> closeables) {
this.protocolSchedule = protocolSchedule;
this.protocolContext = protocolContext;
this.ethProtocolManager = ethProtocolManager;
Expand All @@ -79,7 +86,7 @@ public class BesuController<C> implements java.io.Closeable {
this.transactionPool = transactionPool;
this.miningCoordinator = miningCoordinator;
this.privacyParameters = privacyParameters;
this.close = close;
this.closeables = closeables;
}

public ProtocolContext<C> getProtocolContext() {
Expand Down Expand Up @@ -120,7 +127,15 @@ public MiningCoordinator getMiningCoordinator() {

@Override
public void close() {
close.run();
closeables.forEach(this::tryClose);
}

private void tryClose(final Closeable closeable) {
try {
closeable.close();
} catch (IOException e) {
LOG.error("Unable to close resource.", e);
}
}

public PrivacyParameters getPrivacyParameters() {
Expand Down
53 changes: 10 additions & 43 deletions besu/src/main/java/org/hyperledger/besu/controller/BesuControllerBuilder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@
import org.hyperledger.besu.ethereum.worldstate.WorldStateArchive;
import org.hyperledger.besu.metrics.ObservableMetricsSystem;

import java.io.Closeable;
import java.io.File;
import java.io.IOException;
import java.math.BigInteger;
Expand All @@ -63,16 +64,8 @@
import java.util.Map;
import java.util.Optional;
import java.util.OptionalLong;
import java.util.concurrent.Executors;

import com.google.common.util.concurrent.ThreadFactoryBuilder;
import org.apache.logging.log4j.LogManager;
import org.apache.logging.log4j.Logger;

public abstract class BesuControllerBuilder<C> {

private static final Logger LOG = LogManager.getLogger();

protected GenesisConfigFile genesisConfig;
SynchronizerConfiguration syncConfig;
EthProtocolConfiguration ethereumWireProtocolConfiguration;
Expand All @@ -87,7 +80,6 @@ public abstract class BesuControllerBuilder<C> {
protected boolean isRevertReasonEnabled;
GasLimitCalculator gasLimitCalculator;
private StorageProvider storageProvider;
private final List<Runnable> shutdownActions = new ArrayList<>();
private boolean isPruningEnabled;
private PruningConfiguration pruningConfiguration;
Map<String, String> genesisConfigOverrides;
Expand Down Expand Up @@ -238,27 +230,9 @@ public BesuController<C> build() {
storageProvider.createPruningStorage(),
metricsSystem),
blockchain,
Executors.newSingleThreadExecutor(
new ThreadFactoryBuilder()
.setDaemon(true)
.setPriority(Thread.MIN_PRIORITY)
.setNameFormat("StatePruning-%d")
.build()),
pruningConfiguration));
}

final Optional<Pruner> finalMaybePruner = maybePruner;
addShutdownAction(
() ->
finalMaybePruner.ifPresent(
pruner -> {
try {
pruner.stop();
} catch (final InterruptedException ie) {
throw new RuntimeException(ie);
}
}));

final boolean fastSyncEnabled = syncConfig.getSyncMode().equals(SyncMode.FAST);
final EthProtocolManager ethProtocolManager =
createEthProtocolManager(
Expand Down Expand Up @@ -304,6 +278,13 @@ public BesuController<C> build() {

final JsonRpcMethodFactory additionalJsonRpcMethodFactory =
createAdditionalJsonRpcMethodFactory(protocolContext);

List<Closeable> closeables = new ArrayList<>();
closeables.add(storageProvider);
if (privacyParameters.getPrivateStorageProvider() != null) {
closeables.add(privacyParameters.getPrivateStorageProvider());
}

return new BesuController<>(
protocolSchedule,
protocolContext,
Expand All @@ -315,19 +296,9 @@ public BesuController<C> build() {
transactionPool,
miningCoordinator,
privacyParameters,
() -> {
shutdownActions.forEach(Runnable::run);
try {
storageProvider.close();
if (privacyParameters.getPrivateStorageProvider() != null) {
privacyParameters.getPrivateStorageProvider().close();
}
} catch (final IOException e) {
LOG.error("Failed to close storage provider", e);
}
},
additionalJsonRpcMethodFactory,
nodeKeys);
nodeKeys,
closeables);
}

protected void prepForBuild() {}
Expand All @@ -342,10 +313,6 @@ protected SubProtocolConfiguration createSubProtocolConfiguration(
return new SubProtocolConfiguration().withSubProtocol(EthProtocol.get(), ethProtocolManager);
}

final void addShutdownAction(final Runnable action) {
shutdownActions.add(action);
}

protected abstract MiningCoordinator createMiningCoordinator(
ProtocolSchedule<C> protocolSchedule,
ProtocolContext<C> protocolContext,
Expand Down
13 changes: 5 additions & 8 deletions ...integration-test/java/org/hyperledger/besu/ethereum/worldstate/PrunerIntegrationTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@
import org.hyperledger.besu.ethereum.storage.keyvalue.WorldStatePreimageKeyValueStorage;
import org.hyperledger.besu.ethereum.trie.MerklePatriciaTrie;
import org.hyperledger.besu.ethereum.trie.StoredMerklePatriciaTrie;
import org.hyperledger.besu.ethereum.worldstate.Pruner.PruningPhase;
import org.hyperledger.besu.metrics.noop.NoOpMetricsSystem;
import org.hyperledger.besu.services.kvstore.InMemoryKeyValueStorage;
import org.hyperledger.besu.testutil.MockExecutorService;
Expand Down Expand Up @@ -107,8 +108,8 @@ private void testPruner(
new Pruner(
markSweepPruner,
blockchain,
new MockExecutorService(),
new PruningConfiguration(blockConfirmations, numBlocksToKeep));
new PruningConfiguration(blockConfirmations, numBlocksToKeep),
MockExecutorService::new);

pruner.start();

Expand All @@ -119,13 +120,9 @@ private void testPruner(
var fullyMarkedBlockNum = cycle * numBlockInCycle + 1;

// This should cause a full mark and sweep cycle
assertThat(pruner.getState()).isEqualByComparingTo(Pruner.State.IDLE);
assertThat(pruner.getPruningPhase()).isEqualByComparingTo(PruningPhase.IDLE);
Copy link
Contributor

Choose a reason for hiding this comment

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

State->Pruning phase, while a good change, feels out of scope for the refactor purpose and feels like it should be it's own PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed the name to avoid a conflict with the new State enum that now manages the lifecycle state of the Pruner.

generateBlockchainData(numBlockInCycle, accountsPerBlock);
assertThat(pruner.getState()).isEqualByComparingTo(Pruner.State.IDLE);

// Restarting the Pruner shouldn't matter since we're idle
pruner.stop();
pruner.start();
assertThat(pruner.getPruningPhase()).isEqualByComparingTo(PruningPhase.IDLE);

// Collect the nodes we expect to keep
final Set<BytesValue> expectedNodes = new HashSet<>();
Expand Down
2 changes: 2 additions & 0 deletions ethereum/core/src/main/java/org/hyperledger/besu/ethereum/core/Synchronizer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,8 @@ public interface Synchronizer {

void stop();

void awaitStop() throws InterruptedException;

/**
* @return the status, based on SyncingResult When actively synchronizing blocks, alternatively
* empty
Expand Down
Loading