-
-
Notifications
You must be signed in to change notification settings - Fork 130
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
Couple crashes with java.util.ConcurrentModificationException
and java.util.NoSuchElementException: Key null is missing in the map
#736
Comments
Hello. I did some stress testing and I haven't experienced this crash.
This shouldn't be the reason. Have you tried putting
It's hard to tell without specific information. Is there anything common between the incidents? Like low-spec devices?
I can't see any issues that could cause this. |
I also got this crash, spent a few hours looking for the culprit no luck yet in the below code, in MutableChartValues.kt
UPDATE: I was using the chart host that takes in a CartesianChartModel instead of the CartesianChartModelProducer. After changing it to match OP's implementation with the model producer and the LaunchedEffect, I no longer see the issue, so I'm not sure why OP has the issue. 🤷 |
Hey! Thanks for the reply
Yes, looking further into it, it seems like putting it in the ViewModel would most likely solve it. We were just trying to avoid it since we wanted the chart implementation to be part of a UI-only module, and this way we'll also have to put it a Vico dependency in the module that the ViewModel is in. Or we will have to create a wrapper around it. Reason we wanted to keep it out of the ViewModel is in case we'd want to change the implementation or library, we would have fewer places to change and it would be more contained.
So far only had issues with Android 12, 13, and 14. Devices are not low-spec, they're decent (most common so far is Galaxy A53 5G). I tried to understand the internal Vico code and had an idea, but might be totally wrong. Maybe still worth checking on your side just in case though. Click here to check the detailsI was taking a look at the logic in To solve it, registering or unregistering a receiver would need to either:
Otherwise there might be an unfortunate timing. With that said, I still can't replicate the issue, so I'm probably still missing something! I think I will try to move it to the ViewModel and see if it's fixed, but I believe the race condition will still be possible, just less probable (almost impossible, I guess) since the transaction will most likely be done before the chart is composed, and as such the receiver will always be registered after the transaction is complete (and fewer transactions will also be done, since it will not be done every time the chart is composed), and there's a smaller chance of a transaction being ran when unregistering since there will be fewer transactions too. |
@L-Andrade, Vico 2.0.0-alpha.22 fixes the registration problem you described. This should fix the @ibcurly, thanks for reporting it. Unfortunately, we weren't able to reproduce it, so we'll need a MRE to fix this. |
Hey @Gowsky! Thanks for the update. I took a look and it looks like a nice improvement. In the meantime, we had already shipped the recommended fix of putting the transaction call in the ViewModel (we abstracted it away in a class) which seemed to fix the issue, and it makes sense since it causes much fewer transactions. However, I saw the recommendation in the Vico docs asking to change the dispatcher to Default on the caller side. I believe this should be something done in the Vico side, following the guideline that suspend functions should be main safe from the caller: Other big libraries also respect this guideline, such as Room and Retrofit. This would be a further improvement, a bit less error-prone for the library users Thanks again! |
Thanks for the update and the suggestion, @L-Andrade! Vico 2.0.0 Alpha 23 makes While, as mentioned by @Gowsky, we haven’t been able to reproduce the |
Thanks both! I also believe the changes fix the issue. We will update to that version when we can. Feel free to close the issue! |
Since Vico 1 is still supported, we keep bug reports open until they're addressed in both Vico versions. The "Done in alpha" label basically means that the issue is closed as far as Vico 2 is concerned. |
Just tried
|
Hello, @mak1nt0sh. Can you share more details and some context for the issue? Can you provide an MRE? |
How to reproduce
Hey! Thanks again for the library. It's been working great so far, but we've started getting a couple crashes now after pushing it to more users (nothing major yet).
I've tried to reproduce it for the last couple days, but haven't managed to.
I have a
LazyColumn
with aCartesianChartHost
. This is the basic gist of the chart:I have removed a few bits for brevity and because I can not share the whole code. Let me know if any of it is important to figuring this issue.
The chart is only shown if there are entries to display (
entries
is not empty). It seems to happen after the user scrolls to the bottom of the screen (chart is at the top).Could it be because the
CartesianChartModelProducer
is recreated when the chart is visible again? Since it's in aLazyColumn
, the chart leaves the composition and then returns once the user sees it again. Not sure it should matter though, since the chart is also a "new" one.I'm not entirely sure if it is an issue on our end or an issue in Vico's end. Do you folks have any idea of how we can try to replicate this issue?
Let me know if you see any issue in our implementation too, please!
Observed behavior
There are a few crashes. I have a couple stack traces, not sure if they're linked to the same issue or not
and
Expected behavior
The app should not crash
Vico version(s)
2.0.0-alpha.20
Android version(s)
12, 13, 14
Additional information
We're actually on
2.0.0-alpha.19
, but there seem to be only small changes inalpha.20
.It's quite hard to figure out where the issue is coming from. Since the stack trace does not point to our app code at all, we don't know if it's an issue with our integration or an internal Vico issue.
And since we're not familiar with Vico's codebase, it's a bit harder to try to replicate it. I've tried scrolling up and down multiple times, adding delays in multiple places, adding more charts to the screen and scrolling, configuration changes, going to the background/foreground, spamming buttons that cause the entries to change, navigating back or to another screen, and more. Let me know if you have any ideas that could replicate it, any help would be great here!
Thanks in advance
The text was updated successfully, but these errors were encountered: