[PLAT-6223] Fix unreliable ordering of breadcrumbs #1049
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
nickdowell
force-pushed
the
nickdowell/fix-breadcrumbs-ordering
branch
from
4363667 to
bcbd82f
Compare
nickdowell
force-pushed
the
nickdowell/fix-breadcrumbs-ordering
branch
3 times, most recently
from
36f6395 to
6e0f437
Compare
kattrali
approved these changes
Mar 22, 2021
nickdowell
force-pushed
the
nickdowell/fix-breadcrumbs-ordering
branch
from
6e0f437 to
a0166e1
Compare
|
Not sure what's happened here, I didn't close the PR 🤨 |
kattrali
approved these changes
Mar 22, 2021
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Goal
Fixes a customer reported issue where breadcrumbs sometimes appeared in the wrong order.
Changeset
The bug was caused by sorting the breadcrumb filenames using
-[NSString compare:]- which causes e.g. "20" to appear before "3".-[BugsnagBreadcrumbs loadBreadcrumbsAsDictionaries:]now sorts filenames using a custom comparator.There is remaining issue - when leaving breadcrumbs simultaneously from multiple threads, the order of file writes does not always match the timestamps (a thread may stop executing between recording the timestamp and writing the file) so the timestamps sometimes are out of order.
Attempted sorting breadcrumbs by their timestamp, but that is problematic due to the limited accuracy used in the JSON representation - causing unit tests to fail due to unreliable ordering of breadcrumbs.
Testing
Reproduced the issue by amending the BugsnagStressTest fixture to check breadcrumbs ordering. I have not committed this check since it fails due to the concurrency issue.
Reproducing via E2E tests was not successful as they do not log a sufficient volume of breadcrumbs to trigger the bug.