Skip to content

Picasso.with() should be synchronized#520

Closed
Macarse wants to merge 1 commit into
square:masterfrom
Macarse:macarse/threadSafeSingleton
Closed

Picasso.with() should be synchronized#520
Macarse wants to merge 1 commit into
square:masterfrom
Macarse:macarse/threadSafeSingleton

Conversation

@Macarse
Copy link
Copy Markdown
Contributor

@Macarse Macarse commented May 30, 2014

I was using Picasso 2.2.0 with OkHttp 1.3.0 and everything was working fine.
I updated OkHttp to 1.5.4 and I started seeing instances of square/okhttp#680

After researching a bit I noticed that I had two Picasso instances in my app generated by a race condition in the method that generates the Picasso singleton. I am using Picasso from the main thread but also from a background service to prefetch images.

Apparently I had this bug before but, as @swankjesse explains in square/okhttp#650, the exception was masked by the fact that the cleanup callable was being run with a Future.

@JakeWharton
Copy link
Copy Markdown
Collaborator

This should use double-checked locking. You're optimizing for the worst case instead of the best case.

@Macarse
Copy link
Copy Markdown
Contributor Author

Macarse commented May 30, 2014

@JakeWharton:

  public static Picasso with(Context context) {
    if (singleton == null) {
      synchronized (Picasso.class) {
        if (singleton == null) {
          singleton = new Builder(context).build();
        }
      }
    }
    return singleton;
  }

@JakeWharton
Copy link
Copy Markdown
Collaborator

Yeah. That looks good! Then we skip the lock and the runtime branching strategy will favor the non-null case over time and the method call becomes almost free.

@dnkoutso dnkoutso modified the milestone: Picasso 2.4 May 30, 2014
@Macarse
Copy link
Copy Markdown
Contributor Author

Macarse commented May 30, 2014

@JakeWharton: Good point. I will create a second pull request with the fix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants