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

Use consistent device ID for multi process apps #1013

Conversation

fractalwrench
Copy link
Contributor

@fractalwrench fractalwrench commented Nov 27, 2020

Goal

This changeset makes the notifier use a consistent device ID for apps which initialize Bugsnag in multiple processes. This is achieved by storing the device ID in a single file location which can be accessed concurrently from each process and is protected by a FileLock.

Changeset

  • Created DeviceIdStore which is responsible for loading/saving the device ID to the file system
  • Guarded the device ID file with a FileLock to prevent concurrent access in multiple processes, and a ReadWriteLock to prevent concurrent access from multiple threads.
  • Updated the Client to persist a device ID to a file and to migrate any existing device ID from SharedPreferences or generating a random UUID if none already exists
  • Created SharedPrefMigrator which reads legacy information from SharedPreferences. A separate PR will build on this by making this class read user information too, and delete the SharedPreferences file once the migration is complete
  • Removed responsibility for storing the device ID from UserRepository
  • Updated SynchronizedStreamableStore to use buffered IO

Testing

Added unit test coverage for new classes, and to verify that a file lock is used. Relied on existing E2E test coverage for regression testing, and also manually tested to verify that the device ID is stored in the new location.

@bugsnagbot
Copy link
Collaborator

bugsnagbot commented Nov 30, 2020

Android notifier sizes

Format Size impact of Bugsnag (kB) Size impact of Bugsnag when Minified (kB)
APK 1444.76 1361.31
arm64_v8a 373.35 287.33
armeabi 352.86 266.84
armeabi_v7a 336.49 250.47
x86 410.2 328.28
x86_64 393.82 307.8

Generated by 🚫 Danger

@fractalwrench fractalwrench changed the title Allow persisting consistent device ID in multi process apps Persist consistent device ID for multi process apps Nov 30, 2020
@fractalwrench fractalwrench changed the title Persist consistent device ID for multi process apps Use consistent device ID for multi process apps Nov 30, 2020
@fractalwrench fractalwrench force-pushed the PLAT-5404/device-id-persistence branch 3 times, most recently from 708d23a to 087e8ad Compare November 30, 2020 15:00
@fractalwrench fractalwrench marked this pull request as ready for review November 30, 2020 16:06
} else {
return persistNewDeviceUuid(uuidProvider)
}
} catch (exc: Throwable) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there good cause to catch Throwable here? I would be keen to avoid this and catch more specific errors as we don't catch throwables elsewhere in the codebase. (Also the catch in persistNewDeviceUuid is dong the same an so seem redundant.)

@fractalwrench fractalwrench merged commit e988bb5 into integration/road-958-multi-process Dec 1, 2020
@fractalwrench fractalwrench deleted the PLAT-5404/device-id-persistence branch December 1, 2020 12:54
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