Skip to content
This repository has been archived by the owner on Jun 19, 2023. It is now read-only.

TestHasIsBloomCached failures #6

Closed
Stebalien opened this issue Jul 18, 2018 · 3 comments · Fixed by #9
Closed

TestHasIsBloomCached failures #6

Stebalien opened this issue Jul 18, 2018 · 3 comments · Fixed by #9
Assignees

Comments

@Stebalien
Copy link
Member

Moved from: ipfs/kubo#3208

--- FAIL: TestHasIsBloomCached (0.03s)
    bloom_cache_test.go:100: Bloom filter has cache miss rate of more than 5%
FAIL
FAIL    github.com/ipfs/go-ipfs/blocks/blockstore   0.053s
@taylormike
Copy link
Contributor

taylormike commented Aug 5, 2018

I believe I found the root cause for this test failure. It appears to be a race condition/ timing issue that sometimes causes 'BloomActive()'s' Goroutine to read b.active prior to b.active's assignment via 'Rebuild()'s' Goroutine.

I was able to consistently reproduce this test failure after adding a 300 Microsecond sleep in Rebuild.
(See code snippet below)

@Stebalien @Kubuxu @lgierth, @whyrusleeping Thoughts?

Rebuild's() Goroutine:

        close(b.rebuildChan)
	time.Sleep(300 * time.Microsecond) //<-- Temp Code that reproduces the test failure
	atomic.StoreInt32(&b.active, 1)

BloomActive's() Goroutine:

func (b *bloomcache) BloomActive() bool {
	return atomic.LoadInt32(&b.active) != 0
}

@whyrusleeping
Copy link
Member

@taylormike does it happen if you put that sleep specifically there? Or does it also work if you put the sleep before the channel close?

@taylormike
Copy link
Contributor

@whyrusleeping

  1. The test fails if I put the sleep specifically there. (one sleep)
  2. The test passes if I put the sleep before the channel close. (one sleep)
  3. The test fails if I put the sleep there and I put the sleep before the channel close. (two sleeps)

@Stebalien Stebalien self-assigned this Aug 6, 2018
Stebalien added a commit that referenced this issue Aug 6, 2018
Also stop exposing functions that don't do what they claim to do:

* Invalidate does, technically invalidate. However, it's not threadsafe.
* Rebuild doesn't *clear* the filter so it's only useful for the initial build.
  It's also not threadsafe.

We can restore these functions later if we need them (but we'll have to change a
few things to make them work properly).

Also adds a `Wait` function to allow waiting for the bloom filter to finish
building.

fixes #6
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants