search

facebookarchive / network-connection-class

Listen to current network traffic in the app and categorize the quality of the network.
Other
3.18k stars 515 forks source link

Added double check locking #2

Closed Macarse closed 9 years ago

SeyelentEco commented 9 years ago

Thanks for catching this, however I don't think double check locking is the right approach for this. We should go with the Initialization-on-demand holder idiom since it was developed specifically for lazy instantiation of singletons on Java.

Also, for future reference, the variable must be declared volatile or double check locking won't work correctly. See: http://en.wikipedia.org/wiki/Double-checked_locking

Macarse commented 9 years ago

@SeyelentEco: Whatever you guys prefer using.

In my app I call network-connection-class and device-year-class from different threads and I noticed more than one instance. I proposed the same change to device-network-class (https://github.com/facebook/device-year-class/pull/2) and it was accepted.

I forked both libraries with this fix in and I don't see the problem any more.

SeyelentEco commented 9 years ago

It was an oversight in the device-year-class pull request (the variable not being volatile). We'll fix it there and implement the Initialization-on-demand holder idiom here.

Macarse commented 9 years ago

@SeyelentEco go for it! If you release after fixing I would happily kill my forks :)

SeyelentEco commented 9 years ago

Fixed in https://github.com/facebook/network-connection-class/pull/3 . Will create new release soon.