Skip to content
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

Prevent NullPointerException by not setting the static instance #2061

Merged
merged 1 commit into from
Jul 6, 2017

Conversation

Codeintheory
Copy link

until registration is complete

Description

The Glide instance is not completely thread safe as another thread may get an instance which have not had the components registered

Motivation and Context

With the current code, Thread 2 may get a glide instance because this.glide != null, but registration in Thread 1 has not completed yet

Thread 1- (Does initialization)
GlideApp.with(context).load().into(imageView);

Thread 2-
Loader loader = Glide.get(context).getLoaderFactory().buildModelLoader(modelClass, resourceClass);
Glide.with(mAppContext)
.using(modelLoader, OrbResource.class)
.from(OrbModel.class)

06-21 14:54:44.301: E/AndroidRuntime(29121): java.lang.NullPointerException: ModelLoader must not be null
06-21 14:54:44.301: E/AndroidRuntime(29121): at com.bumptech.glide.provider.FixedLoadProvider.(FixedLoadProvider.java:28)
06-21 14:54:44.301: E/AndroidRuntime(29121): at com.bumptech.glide.GenericTranscodeRequest.build(GenericTranscodeRequest.java:42)
06-21 14:54:44.301: E/AndroidRuntime(29121): at com.bumptech.glide.GenericTranscodeRequest.(GenericTranscodeRequest.java:60)
06-21 14:54:44.301: E/AndroidRuntime(29121): at com.bumptech.glide.RequestManager$GenericModelRequest$GenericTypeRequest.as(RequestManager.java:768)
etc...

@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.


  • If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
  • If your company signed a CLA, they designated a Point of Contact who decides which employees are authorized to participate. You may need to contact the Point of Contact for your company and ask to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the project maintainer to go/cla#troubleshoot.
  • In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.003%) to 83.348% when pulling 71f3c8f on Codeintheory:init into 0349ac3 on bumptech:3.0.

@Codeintheory
Copy link
Author

Completed the license agreement

@TWiStErRob TWiStErRob requested a review from sjudd June 21, 2017 23:08
Copy link
Collaborator

@TWiStErRob TWiStErRob left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice catch!

@Codeintheory
Copy link
Author

The same bug is in the 4.0 branch as well as I see two, btw

@sjudd
Copy link
Collaborator

sjudd commented Jun 21, 2017

Nice catch, thanks!

We'll either have to change the interface in Glide v4 or do something more complicated since the Glide object isn't passed in to the Module in v4.

@sjudd
Copy link
Collaborator

sjudd commented Jun 21, 2017

Also license agreement still seems to be pending.

@TWiStErRob
Copy link
Collaborator

@sjudd same fix would work, all we need is glide = initGlide(context); so the read/write/sync is in the same block and not a side effect in the middle of a method. Or you would expect (=allow?!) registerComponents to call Glide.get(context)?

@Codeintheory
Copy link
Author

I changed commit to include email as per the requirements of the CLA. Waiting for pending license agreement. Thx

@coveralls
Copy link

Coverage Status

Coverage increased (+0.003%) to 83.348% when pulling 2f1820c on Codeintheory:init into 0349ac3 on bumptech:3.0.

@sjudd
Copy link
Collaborator

sjudd commented Jun 26, 2017

@Codeintheory can you double check the cla? Not sure what's up with the bot, but when I look for the email address in the commit or your username it doesn't find anything. Sorry for the inconvenience...

@sjudd
Copy link
Collaborator

sjudd commented Jul 5, 2017

I'd love to get this in, but still waiting on the cla, @Codeintheory is there some other email address that I should check?

@Codeintheory
Copy link
Author

@sjudd I've updated my google CLA to match the email with github account. Please check again for [email protected]. Thx

@googlebot
Copy link

CLAs look good, thanks!

@sjudd sjudd merged commit 211c4db into bumptech:3.0 Jul 6, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants