Skip to content

Conversation

@lucassaldanha
Copy link
Member

PR Description

  • Prevents concurrency issue on LocalNodeRecordStore when checking and comparing the latest record;
  • Updated logic to only notify the record listener updates when the record is actually updated (address or custom field);
  • Updated unit tests to reflect changes in listener update;

Fixed Issue(s)

fixes #190

recordStore.onCustomFieldValueChanged("fieldName", Bytes.fromHexString("0x111111"));

// No updates propagated to listener
assertThat(listenerCalls).hasSize(0);
Copy link
Contributor

Choose a reason for hiding this comment

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

isEmpty

Copy link
Contributor

@rolfyone rolfyone left a comment

Choose a reason for hiding this comment

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

LGTM

if (!record.equals(oldRecord)) {
recordListener.recordUpdated(oldRecord, record);
newRecord -> {
final NodeRecord oldRecord = latestRecord.getAndSet(newRecord);
Copy link
Contributor

Choose a reason for hiding this comment

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

Does it still look as non-atomic update here? get and getAndSet are not under lock and the value can be modified between these calls.

Copy link
Member Author

Choose a reason for hiding this comment

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

Haven't looked into this again. Busy getting PeerDAS into master :)

Will have a second look soon!

@lucassaldanha
Copy link
Member Author

Gonna revisit this one soon! Had a better idea :)

@github-actions github-actions bot locked and limited conversation to collaborators Sep 30, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

LocalNodeRecordStore possible concurrency issue when updated it on DiscoveryManagerImpl

3 participants