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.

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

🐛 Copy the result data instead of referencing directly #113

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

AlexV525
Copy link

When a const map was delivered in the call result, modifying the data would be unsupported since the data is unmodifiable.

result.data['observerIntPtr'] = IrisEventHandlerHandle(eventHandlerIntPtr);

The above line might throw:

Unhandled Exception: Unsupported operation: Cannot modify unmodifiable map

@AlexV525
Copy link
Author

cc @littleGnAl

@littleGnAl
Copy link
Contributor

Thanks @AlexV525, I'm no longer working for Agora. I think the new maintainer will take a look at your PR.
BTW, I'm not sure if you encountered any issues, but I think this error might not occur based on the current implementation.

@AlexV525
Copy link
Author

BTW, I'm not sure if you encountered any issues, but I think this error might not occur based on the current implementation.

For maintainers: the error will occur when an unexpected Agora exception is thrown and the native api result is an empty map.

@littleGnAl
Copy link
Contributor

I think this is an error that could theoretically occur, but has not actually happened, right?

The data is returned here:

return CallApiResult(irisReturnCode: irisReturnCode, data: const {});

return CallApiResult(irisReturnCode: -1, data: const {});

I would personally suggest the following fixes:

  1. Not to return a const map for these cases rather than make a copy for the data.
  2. The CallApiResult should be designed as an immutable class, rather than assign the new value to the map directly, it's better to make a copy of the result , e.g.,
  CallApiResult createNativeEventHandler(IrisMethodCall methodCall) {
        ...
        return CallApiResult(
          irisReturnCode: result.irisReturnCode,
          data: {
            'observerIntPtr': IrisEventHandlerHandle(eventHandlerIntPtr),
            ...result.data
          },
          rawData: result,
        );
  }

Anyway, I think the new maintainer will make a decision about it.

@AlexV525
Copy link
Author

AlexV525 commented Feb 19, 2025

I think this is an error that could theoretically occur, but has not actually happened, right?

Unfortunately, this is an exception I've encountered while debugging.

The CallApiResult should be designed as an immutable class, rather than assign the new value to the map directly, it's better to make a copy of the result

It is, an immutable class does not mean it must be a constant class. But we can take another approach.

@AlexV525
Copy link
Author

AlexV525 commented Mar 7, 2025

cc @peilinok

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.

2 participants