Skip to content

Conversation

@bhavin121
Copy link

Description

This PR modifies the error handling behavior in IndexedCollectionBaseProperty to prevent client crashes when attempting to remove a key that does not exist. Instead of throwing a critical PR-008 error, the system will now log a warning and continue execution.

Problem

  • In high-load scenarios (e.g., 50-100MB data transfer over WebSocket), clients can fall significantly out of sync. This leads to a specific race condition
  • If two clients were out of sync and they have generated identicle remove ops
  • Now one delete op A gets summarized where the other delete op B remain unsummarized.
  • In this case when a new client joins the session will always get PR-008 error.
  • And the document cannot be used at all.

Solution

The _removeByKey method in IndexedCollectionBaseProperty has been updated to:
Warn instead of Throw: When a non-existent key removal is attempted, it now logs console.warn(MSG.REMOVED_NON_EXISTING_ENTRY + in_key) instead of throwing an error.

Breaking Changes

This PR introduces a breaking change to the behavior of remove() methods in PropertyDDS collection types (MapProperty, ReferenceMapProperty, SetProperty).

Previous Behavior

  • Attempting to remove a non-existent key would throw an error: Error: PR-008: Trying to remove a non-existing entry: <key>
  • This would cause the client to crash and stop processing operations

New Behavior

  • Attempting to remove a non-existent key will:
    • Log a warning to the console: PR-008: Trying to remove a non-existing entry: <key>
    • Return undefined (instead of throwing)
    • Continue execution without interruption

Migration Impact

Code that relies on errors being thrown will need to be updated:

// ❌ OLD CODE - Will no longer work as expected
try {
  mapProperty.remove('nonExistentKey');
} catch (error) {
  // This catch block will never execute now
  handleError(error);
}

// ✅ NEW CODE - Check return value instead
const removed = mapProperty.remove('nonExistentKey');
if (removed === undefined) {
  // Key didn't exist
  handleMissingKey();
}

this._setDirty(in_reportToView);
} else {
throw new Error(MSG.REMOVED_NON_EXISTING_ENTRY + in_key);
console.warn(MSG.REMOVED_NON_EXISTING_ENTRY + in_key);
Copy link
Contributor

Choose a reason for hiding this comment

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

fluid should never write directly to the console. there should be a logger somewhere that you can log an error to.

Copy link
Author

Choose a reason for hiding this comment

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

Hi @anthony-murphy, there is no logger being used in PropertyDDS. And only direct console logs are there.

@bhavin121
Copy link
Author

@microsoft-github-policy-service agree company="Autodesk"

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants