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

Attempt Pending Request to BUSY && ASSIGNED && DISCONNECTED Peer #1321

Merged
merged 4 commits into from
Aug 20, 2020
Merged

Attempt Pending Request to BUSY && ASSIGNED && DISCONNECTED Peer #1321

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
05540f9
Attempt pending request to disconnected busy assigned peer
RatanRSur Aug 18, 2020
e39f95c
remove unnecessary line
RatanRSur Aug 18, 2020
e96a76d
remove comment
RatanRSur Aug 18, 2020
ea287e8
Merge branch 'master' into abort-assigned-peer-request
RatanRSur Aug 20, 2020
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
6 changes: 6 additions & 0 deletions ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/EthPeer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@
import java.util.concurrent.atomic.AtomicReference;
import java.util.function.Consumer;

import com.google.common.annotations.VisibleForTesting;
import org.apache.logging.log4j.LogManager;
import org.apache.logging.log4j.Logger;
import org.apache.tuweni.bytes.Bytes;
Expand Down Expand Up @@ -395,6 +396,11 @@ public Set<Capability> getAgreedCapabilities() {
return connection.getAgreedCapabilities();
}

@VisibleForTesting
PeerConnection getConnection() {
return connection;
}

public Bytes nodeId() {
return connection.getPeerInfo().getNodeId();
}
Expand Down
6 changes: 3 additions & 3 deletions ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/EthPeers.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,7 @@ void registerDisconnect(final PeerConnection connection) {
disconnectCallbacks.forEach(callback -> callback.onDisconnect(peer));
peer.handleDisconnect();
}
checkPendingConnections();
reattemptPendingPeerRequests();
}

public EthPeer peer(final PeerConnection peerConnection) {
Expand All @@ -99,11 +99,11 @@ public PendingPeerRequest executePeerRequest(
public void dispatchMessage(final EthPeer peer, final EthMessage ethMessage) {
peer.dispatch(ethMessage);
if (peer.hasAvailableRequestCapacity()) {
checkPendingConnections();
reattemptPendingPeerRequests();
}
}

private void checkPendingConnections() {
private void reattemptPendingPeerRequests() {
synchronized (this) {
pendingRequests.removeIf(PendingPeerRequest::attemptExecution);
}
Expand Down
1 change: 1 addition & 0 deletions ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/RequestManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,7 @@ public void dispatchResponse(final EthMessage message) {

public void close() {
closeOutstandingStreams(responseStreams.values());
outstandingRequests.set(0);
}

private ResponseStream createStream() {
Expand Down
18 changes: 18 additions & 0 deletions ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/manager/EthPeersTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
import static org.assertj.core.api.Assertions.assertThat;
import static org.assertj.core.api.Assertions.fail;
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.verifyZeroInteractions;
Expand Down Expand Up @@ -247,6 +248,23 @@ public void shouldNotExecuteAbortedRequest() throws Exception {
assertRequestFailure(pendingRequest, CancellationException.class);
}

// We had a bug where if a peer was busy when it was disconnected, pending peer requests that were
// *explicitly* assigned to that peer would never be attempted and thus never completed
@Test
public void shouldAttemptRequestWithBusyDisconnectedAssignedPeer() throws Exception {
final RespondingEthPeer peer = EthProtocolManagerTestUtil.createPeer(ethProtocolManager, 1000);
final EthPeer ethPeer = peer.getEthPeer();
useAllAvailableCapacity(ethPeer);

final PendingPeerRequest pendingRequest =
ethPeers.executePeerRequest(peerRequest, 100, Optional.of(ethPeer));

when(peerRequest.sendRequest(eq(ethPeer))).thenThrow(new PeerNotConnected(""));
ethPeers.registerDisconnect(ethPeer.getConnection());

assertRequestFailure(pendingRequest, PeerDisconnectedException.class);
}

private void freeUpCapacity(final EthPeer ethPeer) {
ethPeers.dispatchMessage(ethPeer, new EthMessage(ethPeer, NodeDataMessage.create(emptyList())));
}
Expand Down