Skip to content

Commit

Permalink
[REFACTOR] Use common path for updating map/data store on remove
Browse files Browse the repository at this point in the history
Previously, the expire path, the remove path, and the onDisconnect
all used separate logic for updating the map, signaling listeners, and
removing PersistablePaylod objects from the data store. This led to a
bug where the onDisconnect path did not update the protectedDataStore.

Combine the three code paths to ensure that the same state is updated
regardless of the context.
  • Loading branch information
julianknutsen committed Nov 14, 2019
1 parent bacd28d commit bd14e6a
Showing 1 changed file with 16 additions and 26 deletions.
42 changes: 16 additions & 26 deletions p2p/src/main/java/bisq/network/p2p/storage/P2PDataStorage.java
Original file line number Diff line number Diff line change
Expand Up @@ -199,10 +199,7 @@ void removeExpiredEntries() {
ByteArray payloadHash = mapEntry.getKey();

log.debug("We found an expired data entry. We remove the protectedData:\n\t" + Utilities.toTruncatedString(protectedStorageEntry));
map.remove(payloadHash);

hashMapChangedListeners.forEach(l -> l.onRemoved(protectedStorageEntry));
removeFromProtectedDataStore(protectedStorageEntry);
removeFromMapAndDataStore(protectedStorageEntry, payloadHash);
});
hashMapChangedListeners.forEach(HashMapChangedListener::onBatchRemoveExpiredDataCompleted);

Expand Down Expand Up @@ -289,8 +286,7 @@ public void onDisconnect(CloseConnectionReason closeConnectionReason, Connection
if (protectedStorageEntry.isExpired(this.clock)) {
log.info("We found an expired data entry which we have already back dated. " +
"We remove the protectedStoragePayload:\n\t" + Utilities.toTruncatedString(protectedStorageEntry.getProtectedStoragePayload(), 100));
doRemoveProtectedExpirableData(protectedStorageEntry, hashOfPayload);
removeFromProtectedDataStore(protectedStorageEntry);
removeFromMapAndDataStore(protectedStorageEntry, hashOfPayload);
}
} else {
log.debug("Remove data ignored as we don't have an entry for that data.");
Expand Down Expand Up @@ -505,7 +501,7 @@ public boolean remove(ProtectedStorageEntry protectedStorageEntry,
return false;

// Valid remove entry, do the remove and signal listeners
doRemoveProtectedExpirableData(protectedStorageEntry, hashOfPayload);
removeFromMapAndDataStore(protectedStorageEntry, hashOfPayload);
printData("after remove");

// Record the latest sequence number and persist it
Expand All @@ -520,8 +516,6 @@ public boolean remove(ProtectedStorageEntry protectedStorageEntry,
broadcast(new RemoveDataMessage(protectedStorageEntry), sender, null, isDataOwner);
}

removeFromProtectedDataStore(protectedStorageEntry);

return true;
}

Expand All @@ -543,8 +537,7 @@ public void removeInvalidProtectedStorageEntry(ProtectedStorageEntry protectedSt
return;
}

doRemoveProtectedExpirableData(protectedStorageEntry, hashOfPayload);
removeFromProtectedDataStore(protectedStorageEntry);
removeFromMapAndDataStore(protectedStorageEntry, hashOfPayload);

// We do not update the sequence number as that method is only called if we have received an invalid
// protectedStorageEntry from a previous add operation.
Expand All @@ -557,19 +550,6 @@ public void removeInvalidProtectedStorageEntry(ProtectedStorageEntry protectedSt
// source (network).
}

private void removeFromProtectedDataStore(ProtectedStorageEntry protectedStorageEntry) {
ProtectedStoragePayload protectedStoragePayload = protectedStorageEntry.getProtectedStoragePayload();
if (protectedStoragePayload instanceof PersistablePayload) {
ByteArray compactHash = getCompactHashAsByteArray(protectedStoragePayload);
ProtectedStorageEntry previous = protectedDataStoreService.remove(compactHash, protectedStorageEntry);
if (previous != null) {
protectedDataStoreListeners.forEach(e -> e.onRemoved(protectedStorageEntry));
} else {
log.info("We cannot remove the protectedStorageEntry from the persistedEntryMap as it does not exist.");
}
}
}

private void maybeAddToRemoveAddOncePayloads(ProtectedStoragePayload protectedStoragePayload,
ByteArray hashOfData) {
if (protectedStoragePayload instanceof AddOncePayload) {
Expand Down Expand Up @@ -656,10 +636,20 @@ public void removeProtectedDataStoreListener(ProtectedDataStoreListener listener
// Private
///////////////////////////////////////////////////////////////////////////////////////////

private void doRemoveProtectedExpirableData(ProtectedStorageEntry protectedStorageEntry, ByteArray hashOfPayload) {
private void removeFromMapAndDataStore(ProtectedStorageEntry protectedStorageEntry, ByteArray hashOfPayload) {
map.remove(hashOfPayload);
log.trace("Data removed from our map. We broadcast the message to our peers.");
hashMapChangedListeners.forEach(e -> e.onRemoved(protectedStorageEntry));

ProtectedStoragePayload protectedStoragePayload = protectedStorageEntry.getProtectedStoragePayload();
if (protectedStoragePayload instanceof PersistablePayload) {
ByteArray compactHash = getCompactHashAsByteArray(protectedStoragePayload);
ProtectedStorageEntry previous = protectedDataStoreService.remove(compactHash, protectedStorageEntry);
if (previous != null) {
protectedDataStoreListeners.forEach(e -> e.onRemoved(protectedStorageEntry));
} else {
log.info("We cannot remove the protectedStorageEntry from the persistedEntryMap as it does not exist.");
}
}
}

private boolean hasSequenceNrIncreased(int newSequenceNumber, ByteArray hashOfData) {
Expand Down

0 comments on commit bd14e6a

Please sign in to comment.