We already have a garbage collection thread that runs every minute
to clean up items. Doing it again during onDisconnect is an unnecessary
optimization that adds complexity and caused bugs.
For example, the original implementation did not handle the sequence
number map correctly and was removing entries during a stream iteration.
This also reduces the complexity of testing. There is one code path
responsible for reducing ttls and one code path responsible for
expiring entries. Much easier to reason about.
1. Remove delete during stream iteration
2. Minimize branching w/ early returns for bad states
3. Use stream filter for readability
4. Implement additional checks that should be done when removing entries
Before refactoring the function ensure the tests cover all cases. This
fixes a bug where the payload ttl was too low in some instances causing
backDate to do no work when it should.
isDataOwner is used when deciding how many peer nodes should receive
a BroadcastMessage. If the BroadcastMessage originated
on the local node it is sent to ALL peer nodes with a small delay.
If the node is only relaying the message (it originated on a different
node) it is sent to MAX(peers.size(), 7) peers with a delay that is
twice as long.
All the information needed to determine whether or not the
BroadcastMessage originated on the local node is available at the final
broadcast site and there is no reason to have callers pass it in.
In the event that the sender address is not known during broadcast (which
is only a remote possibility due to how early the local node address
is set during startup) we can default to relay mode.
This first patch just removes the deep parameters. The next will remove
everything else. There is one real change in LiteNodeNetworkService.java
where it was using the local node when it should have been using the
peer node. This was updated to the correct behavior.
Now that more callers have moved internal, the public facing API
can be cleaner and more simple. This should lead to a more maintainable
API and less sharp edges with future development work.
Now that the only user is internal, the API can be made private and the
tests can be removed. This involved adding a few test cases to
processGetDataResponse to ensure the invalid hash size condition was
still covered.
The only two users of this constructor are the fromProto path which
now creates an empty Capabilities object similar to GetDataResponse.
The other internal usage of Capabilities.app which is initialized to empty.
The only two users of this constructor are the fromProto path which
already creates an empty Capabilities object if one is not provided and
the internal usage of Capabilities.app which is initialized to empty.
Remove the @Nullable so future readers aren't confused.
Checking for null creates hard-to-read code and it is simpler to just
create an empty set if we receive a pre-v0.6 GetDataResponse protobuf
message that does not have the field set.
Write a few integration test that exercises the exercise interesting
synchronization states including the lost remove bug. This fails
with the proper validation, but will pass at the end of the new feature
development.
Previously, multiple handlers needed to signal off one global variable.
Now, that this check is inside the singleton P2PDataStorage, make it
non-static and private.
Now that we want to unit test the GetData path which has different
behavior w.r.t. broadcasts, the tests need a way to verify that
state was updated, but not broadcast during an add.
This patch changes all verification function to take each state update
explicitly so the tests can do the proper verification.
Introduce a generic function that can be used to filter
Map<ByteArray, PersistableNetworkPayload> or
Map<ByteArray, ProtectedStorageEntry>.
Used to deduplicate the GetData code paths and ensure the logic is the
same between the two payload types.
Move the capability check inside the stream operation. This should
improve performance slightly, but more importantly it makes the
two filter functions almost identical so they can be combined.
The appendOnlyDataStoreService and map already have unique keys that
are based on the hash of the payload. This would catch instances
where:
PersistableNetworkPayload
- None: The key is based on ByteArray(payload.getHash()) which is the
same as this check.
ProtectedStorageEntry
- Cases where multiple PSEs contain payloads that have equivalent
hashCode(), but different data.toProtoMessage().toByteArray().
I don't think it is a good idea to keep 2 "unique" methods on
payloads. This is likely left over from a time when
Payload hashCode() needed to be different than the hash of
the payload.
Remove the dependence on the connection object by having the handler
pass in the peer's capabilities. This now allows unit testing of
buildGetDataResponse without any connection dependencies.
Move the logging that utilizes connection information into the request
handler. Now, buildGetDataResponse just returns whether or not the list
is truncated which will make it easier to test.
Changed the log to reference getDataResponse instead of getData. Now
that we might truncate the response, it ins't true that this is exactly
what the peer asked.
These are identical test cases to the requestHandler tests, but with much
fewer dependencies. The requestHandler tests will eventually be deleted,
but they are going to remain throughout development as an extra safety
net.
As part of changing the GetData path, we want to move all creation
and processing of GetData messages inside P2PDataStorage. This will allow
easier unit testing of the behavior as well as cleaner code in the
request handlers that can just focus on nonces, connections, etc.
