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
Try to avoid ConcurrentModificationError by not using a Future.forEach #1259
Conversation
Codecov ReportBase: 89.72% // Head: 88.78% // Decreases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## main #1259 +/- ##
==========================================
- Coverage 89.72% 88.78% -0.95%
==========================================
Files 155 114 -41
Lines 5034 3666 -1368
==========================================
- Hits 4517 3255 -1262
+ Misses 517 411 -106 Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
@ueman I added on the description what I think, but I can't reproduce or confirm it. |
Android Performance metrics 🚀
|
Revision | Plain | With Sentry | Diff |
---|---|---|---|
6325c3b | 339.33 ms | 409.86 ms | 70.53 ms |
9f9f94f | 331.04 ms | 368.92 ms | 37.88 ms |
559d28f | 302.35 ms | 339.53 ms | 37.18 ms |
5112c69 | 333.67 ms | 363.74 ms | 30.08 ms |
521cfbf | 332.78 ms | 376.04 ms | 43.26 ms |
fbf42af | 323.04 ms | 365.09 ms | 42.04 ms |
3a69405 | 334.34 ms | 369.19 ms | 34.85 ms |
6d7a391 | 331.94 ms | 367.04 ms | 35.10 ms |
5aba417 | 355.78 ms | 450.39 ms | 94.61 ms |
870f5eb | 329.45 ms | 369.29 ms | 39.84 ms |
App size
Revision | Plain | With Sentry | Diff |
---|---|---|---|
6325c3b | 5.94 MiB | 6.96 MiB | 1.02 MiB |
9f9f94f | 5.94 MiB | 6.95 MiB | 1.01 MiB |
559d28f | 5.94 MiB | 6.92 MiB | 1001.70 KiB |
5112c69 | 5.94 MiB | 6.96 MiB | 1.02 MiB |
521cfbf | 5.94 MiB | 6.97 MiB | 1.03 MiB |
fbf42af | 5.94 MiB | 6.96 MiB | 1.02 MiB |
3a69405 | 5.94 MiB | 6.95 MiB | 1.01 MiB |
6d7a391 | 5.94 MiB | 6.95 MiB | 1.01 MiB |
5aba417 | 5.94 MiB | 6.96 MiB | 1.02 MiB |
870f5eb | 5.94 MiB | 6.92 MiB | 1005.77 KiB |
iOS Performance metrics 🚀
|
Revision | Plain | With Sentry | Diff |
---|---|---|---|
9811573 | 1259.78 ms | 1278.33 ms | 18.55 ms |
f4cc744 | 1274.57 ms | 1290.79 ms | 16.22 ms |
b728df4 | 1287.43 ms | 1293.94 ms | 6.51 ms |
521cfbf | 1258.94 ms | 1275.18 ms | 16.25 ms |
21845e2 | 1279.37 ms | 1298.81 ms | 19.45 ms |
a7acb24 | 1296.71 ms | 1317.69 ms | 20.98 ms |
322aa66 | 1251.68 ms | 1275.52 ms | 23.84 ms |
49a149b | 1296.47 ms | 1320.20 ms | 23.73 ms |
5603ab2 | 1268.47 ms | 1280.73 ms | 12.26 ms |
b47809a | 1252.61 ms | 1278.57 ms | 25.96 ms |
App size
Revision | Plain | With Sentry | Diff |
---|---|---|---|
9811573 | 8.16 MiB | 9.17 MiB | 1.01 MiB |
f4cc744 | 8.16 MiB | 9.16 MiB | 1.01 MiB |
b728df4 | 8.15 MiB | 9.15 MiB | 1020.72 KiB |
521cfbf | 8.16 MiB | 9.17 MiB | 1.01 MiB |
21845e2 | 8.15 MiB | 9.12 MiB | 991.34 KiB |
a7acb24 | 8.16 MiB | 9.17 MiB | 1.01 MiB |
322aa66 | 8.15 MiB | 9.12 MiB | 992.53 KiB |
49a149b | 8.15 MiB | 9.12 MiB | 986.26 KiB |
5603ab2 | 8.15 MiB | 9.12 MiB | 990.57 KiB |
b47809a | 8.16 MiB | 9.17 MiB | 1.01 MiB |
Thanks! The explanation seems plausible to me. Unfortunately, I also don't have any further insights. For me, it seems like there can happen some weird kind of race condition. The fix in this PR is something which solved it for, but I can't really explain why :/ It's really nagging me, that I can't figure this bug out. Even if this doesn't fix the issue, this will improve the stacktrace of the thrown error and thus helpfully get us closer to figuring it out for real. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 🚀
📜 Description
Try to avoid ConcurrentModificationError by not using a
Future.forEach
.I believe that the event loop has a sort of priority queue and maybe the
future
of finishing a late span is actually executed within the items offorEach
(more priority in the scheduling) hence causing theConcurrentModificationError
, this is a hunch, I can't reproduce it either, let's go with a simple for loop for now.💡 Motivation and Context
Closes #1256
💚 How did you test it?
📝 Checklist
sendDefaultPii
is enabled🔮 Next steps