Skip to content

Fix race condition in P1.memo()#106

Merged
mperry merged 1 commit into
functionaljava:masterfrom
knutwalker:bugs/105
Feb 11, 2015
Merged

Fix race condition in P1.memo()#106
mperry merged 1 commit into
functionaljava:masterfrom
knutwalker:bugs/105

Conversation

@knutwalker

Copy link
Copy Markdown
Contributor

The race was when two thread encountered the uninitialized state, a == null.
One thread gets suspended before entering the synchronized block, while the other runs though it. The latter thread sets v to something not null. Then the first thread resumes, enters the synchronized block and checks if (v == null || v.get() == null) which is now false, and thus returns the still uninitialized a.

Fixes #105

The race was when two thread encountered the uninitialized state, `a == null`.
One thread gets suspended before entering the `synchronized` block, while the other runs though it. The latter thread sets `v` to something not null. Then the first thread resumes, enters the `synchronized` block and checks if `(v == null || v.get() == null)` which is now false, and thus returns the still uninitialized `a`.

Fixes functionaljava#105
@mperry

mperry commented Feb 11, 2015

Copy link
Copy Markdown
Contributor

Thanks for your fast response @knutwalker. I read through your reasoning with the code and believe you are correct.

mperry added a commit that referenced this pull request Feb 11, 2015
Fix race condition in P1.memo()
@mperry
mperry merged commit 7ca522d into functionaljava:master Feb 11, 2015
@mperry mperry added this to the v4.3 milestone Feb 11, 2015
@mperry mperry added the bug label Feb 11, 2015
@zot

zot commented Feb 14, 2015

Copy link
Copy Markdown

The code looks to me like the caching doesn't work for a _1() will that returns null, i.e. repeated calls to memo() will repeatedly evaluate _1(), contributing to the quadratic execution time that memos were created to avoid.

Maybe the condition should only be based on v == null -- something like this?

public A _1() {
  if (v != null) return v.get();
  synchronized (latch) {
    if (v != null) return v.get(); //check again here in case v was assigned asynchronously
    A a = self._1();
    v = new SoftReference<A>(a);
    return a;
  }
}

@mperry

mperry commented Feb 16, 2015

Copy link
Copy Markdown
Contributor

I created #108 to deal with this.

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants