PhilipsHue / PhilipsHueSDK-Java-MultiPlatform-Android

The Software Development Kit for Philips Hue Java Mulfi-Platform and Android (beta)
273 stars 214 forks source link

ConcurrentModificationException in the Hue SDK #7

Open ghost opened 10 years ago

ghost commented 10 years ago

We are busy with a Hue app on Android, and often when the bridge becomes unavailable we get the error below. Our implementation of PHSDKListener will post all callbacks to a handler, so we are pretty sure it is not something in our code. Can you please look into it?

Process: <MyProcess>, PID: 10493
java.util.ConcurrentModificationException
at java.util.ArrayList$ArrayListIterator.next(ArrayList.java:573)
at com.philips.lighting.hue.sdk.notification.impl.PHNotificationManagerImpl$1.onReceived(PHNotificationManagerImpl.java:128)
at com.philips.lighting.hue.sdk.notification.impl.PHHueResultReceiver.execute(PHHueResultReceiver.java:26)
at com.philips.lighting.hue.sdk.notification.impl.PHNotificationManagerImpl.notifySDKError(PHNotificationManagerImpl.java:136)
at com.philips.lighting.hue.sdk.heartbeat.PHHeartbeatProcessor.run(PHHeartbeatProcessor.java:132)
at java.util.Timer$TimerImpl.run(Timer.java:284)
SteveyO commented 9 years ago

Firstly apologies for the delay in responding. I have an issue with github notifications but no excuse. Are you still experiencing this problem? If so do you have a reproducible scenario?

I have checked the code and I can't see anywhere which can result in a ConcurrentModificationException. The onReceived method (PHNotification manager) is just calling the onError method in the PHSDKListener (which is created by yourself). If you are still experiencing issues, can you check your onError method (in your PHSDKListener object). Thanks

richardmcclellan commented 9 years ago

I am seeing this issue as well. I'd appreciate it if someone could look into it again, thanks!

SteveyO commented 9 years ago

Sure, I will take another look. If you have any more details, as to what happened exactly to cause the exception it would be helpful.

iagreen commented 9 years ago

@SteveyO I have been debugging this, and I think it occurs when we call registerSDKListener() on the NotificationManager from our app on the main thread while the onError callbacks are being fired on a background thread. Are you traversing the list of listeners on a background thread?

SteveyO commented 9 years ago

@iagreen Thanks, this info was helpful, I have been looking at this issue again. Yes, calling the registerSDKListener adds a listener to a list. And the onError is being fired on a different thread (and this traverses through this list list) so I can see how a concurrentModificationException could be thrown here.

Generally in the SDK, any http call (heartbeat, updates, find new lights etc) is called on a separate thread and the onError is called from this thread.

However, saying that am still a little baffled. Normally, I would expect the registerSDKListener to be called when the app is first created and only once (before any http calls are made and before the connection to the bridge has been established).
Could you please clarify when you are registering the listener?
i.e. Calling phHueSDK.getNotificationManager().registerSDKListener(listener);

And I will take it from there. Sorry I don't have anything more concrete, it's a tricky one this.

iagreen commented 9 years ago

We were accidentally registering the listener way more than we should have been, which is how I noticed where this error came from. That has been fixed on our end. As you say, if we register once then you will never run into the issue, but we do actually register two listeners at different points in our app, so in theory still open to getting this crash.

On Fri, Dec 12, 2014 at 5:05 AM, Steve notifications@github.com wrote:

@iagreen https://github.com/iagreen Thanks, this info was helpful, I have been looking at this issue again. Yes, calling the registerSDKListener adds a listener to a list. And the onError is being fired on a different thread (and this traverses through this list list) so I can see how a concurrentModificationException could be thrown here.

Generally in the SDK, any http call (heartbeat, updates, find new lights etc) is called on a separate thread and the onError is called from this thread.

However, saying that am still a little baffled. Normally, I would expect the registerSDKListener to be called when the app is first created and only once (before any http calls are made and before the connection to the bridge has been established).

Could you please clarify when you are registering the listener?

i.e. Calling phHueSDK.getNotificationManager().registerSDKListener(listener);

And I will take it from there. Sorry I don't have anything more concrete, it's a tricky one this.

— Reply to this email directly or view it on GitHub https://github.com/PhilipsHue/PhilipsHueSDK-Java-MultiPlatform-Android/issues/7#issuecomment-66753704 .