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

Fix bug preventing native SDK from being called after reinitialization #3200

Merged
merged 6 commits into from
Aug 8, 2023

Conversation

mateioprea
Copy link
Contributor

📢 Type of change

  • Bugfix
  • New feature
  • Enhancement
  • Refactoring

📜 Description

This commit addresses a bug in the Sentry React Native integration that occurs when calling Sentry.init({ enableNative: true }), followed by Sentry.close(), and then re-calling Sentry.init({ enableNative: true }). The bug prevents the native SDK from being invoked after the reinitialization.

The issue resides in the closeNativeSdk() method, where the enableNative flag is incorrectly used to determine whether the native SDK should be closed. This flag is not being updated correctly after reinitialization, causing the SDK to remain disabled.

With this change, the nativeIsReady flag is used to determine whether the native SDK should be closed, ensuring that subsequent reinitialization correctly enables the native SDK functionality.

This fix ensures that the native SDK will be called as expected after reinitializing with enableNative: true, resolving the issue where the SDK was not invoked due to the bug in the closeNativeSdk() method.

💡 Motivation and Context

💚 How did you test it?

In my App.

📝 Checklist

  • I reviewed submitted code
  • I added tests to verify changes
  • No new PII added or SDK only sends newly added PII if sendDefaultPII is enabled
  • All tests passing
  • No breaking changes

🔮 Next steps

This commit addresses a bug in the Sentry React Native integration that occurs when calling `Sentry.init({ enableNative: true })`, followed by `Sentry.close()`, and then re-calling `Sentry.init({ enableNative: true })`. The bug prevents the native SDK from being invoked after the reinitialization.

The issue resides in the closeNativeSdk() method, where the enableNative flag is incorrectly used to determine whether the native SDK should be closed. This flag is not being updated correctly after reinitialization, causing the SDK to remain disabled.

With this change, the nativeIsReady flag is used to determine whether the native SDK should be closed, ensuring that subsequent reinitialization correctly enables the native SDK functionality.

This fix ensures that the native SDK will be called as expected after reinitializing with enableNative: true, resolving the issue where the SDK was not invoked due to the bug in the closeNativeSdk() method.
@krystofwoldrich
Copy link
Member

Hi,
thank you for the details and for informing us about the regression,

I believe the fix should be in the isNativeAvailable and not in the closeMethod.

The closeMethod correctly flips the enableNative flag. But isNativeAvailable should not read from it.

@mateioprea
Copy link
Contributor Author

Fair enough! I will change it there

@mateioprea
Copy link
Contributor Author

@krystofwoldrich done!

@krystofwoldrich
Copy link
Member

@mateioprea Looks good, I'm thinking about some tests to prevent this in the future, but nothing comes to mind.

Can you update the changelog?

@mateioprea mateioprea force-pushed the mateioprea-patch-2 branch from b8199ef to 073ded8 Compare July 20, 2023 17:04
@mateioprea
Copy link
Contributor Author

@krystofwoldrich done. I had my previous PR opened and I made a mistake in the CHANGELOG file, but i fixed it.

I cannot think of any test cases for now as well.

Thank you!

@mateioprea
Copy link
Contributor Author

@krystofwoldrich any news here? :D

Copy link
Member

@krystofwoldrich krystofwoldrich left a comment

Choose a reason for hiding this comment

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

Thank you for the detailed information and the fix. Looks good. 🚀

Tested with the sample app.

@krystofwoldrich krystofwoldrich merged commit 538cedb into getsentry:main Aug 8, 2023
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