Get: Threading issue due to direct call to memCache

[PaulWoitaschek]

For me the store does not work.

If I understand correctly multiple calls to get() with the same barcode should only subscribe to the source observable once. But that only works if I make the calls strictly sequentially. When I call get from different places, it makes a new network call for each. This is because you immediately call

memCache.get(barCode, new Callable<Observable<Parsed>>() from

return Observable.concat(
                cache(barCode),
                fetch(barCode)
        ).take(1);

in get. Here is a failing testcase

import com.nytimes.android.external.store.base.Store
import com.nytimes.android.external.store.base.impl.BarCode
import com.nytimes.android.external.store.base.impl.StoreBuilder
import org.junit.Before
import org.junit.Test
import rx.Observable

class Test {

    var networkCalls = 0
    lateinit var store: Store<Unit>

    @Before fun setup() {
        networkCalls = 0
        store = StoreBuilder.builder<Unit>()
                .fetcher {
                    Observable.fromCallable {
                        networkCalls++
                        Unit
                    }
                }
                .open()
    }

    @Test fun sequentially() {
        val b = BarCode("one", "two")
        store.get(b).test().awaitTerminalEvent()
        store.get(b).test().awaitTerminalEvent()

        check(networkCalls == 1) { "there are $networkCalls networkCalls" }
    }

    @Test fun parallel() {
        val b = BarCode("one", "two")
        val first = store.get(b)
        val second = store.get(b)

        first.test().awaitTerminalEvent()
        second.test().awaitTerminalEvent()

        check(networkCalls == 1) { "there are $networkCalls networkCalls" }
    }
}

[digitalbuddha]

Top of the morning! Could you try with a non empty barcode. Also are you using the latest version, there was a missing hash code previously. The way the functionality is expected to work: memory caching is done in a Guava LoadingCache which backs each store. If multiple requests are made to the same mem cache, the loader will block each subsequent request until a single result is made. I'll validate if a bug. Thanks for reporting

[PaulWoitaschek]

Same with non-empty barcodes. Tested with be47cff
Adjusted the provided testcase above.

[digitalbuddha]

Thanks for reporting. Will look at it today. I'm curious why StoreTest.testDoubleTap is passing yet this is not. Yay for more tests ☺️

[digitalbuddha]

Hey 1 more request. Could you please convert the test case to Java. I want to make sure I test against exactly what is failing for you

[PaulWoitaschek]

Can't you add kotlin for tests? Java is such a pain

import com.nytimes.android.external.store.base.Store;
import com.nytimes.android.external.store.base.impl.BarCode;
import com.nytimes.android.external.store.base.impl.StoreBuilder;
import org.junit.Before;
import org.junit.Test;
import rx.Observable;

import static org.assertj.core.api.Assertions.assertThat;

/**
 * @author Paul Woitaschek
 */
public class SequentialTest {

    private int networkCalls = 0;
    private Store<Integer> store;

    @Before
    public void setup() {
        networkCalls = 0;
        store = StoreBuilder.<Integer>builder()
                .fetcher(barCode -> Observable.fromCallable(() -> networkCalls++))
                .open();
    }

    @Test
    public void sequentially() {
        BarCode b = new BarCode("one", "two");
        store.get(b).test().awaitTerminalEvent();
        store.get(b).test().awaitTerminalEvent();

        assertThat(networkCalls).isEqualTo(1);
    }

    @Test
    public void parallel() {
        BarCode b = new BarCode("one", "two");
        Observable<Integer> first = store.get(b);
        Observable<Integer> second = store.get(b);

        first.test().awaitTerminalEvent();
        second.test().awaitTerminalEvent();

        assertThat(networkCalls).isEqualTo(1);
    }
}

[digitalbuddha]

you nailed it.

This is because you immediately call

Wrapped the call in a defer and tests pass. Thank you for using (and finding) issues within the Store. LMK if ever in nyc, I owe you a beer (and a visit to ny times if you'd like)

[digitalbuddha]

Ok so you're find actually surfaced a few issues. Here is another one: The disk cache was not lazy. Internally this was fine for us, but for you without a disk cache the No-Op disk cache internally was immediately encoding a cache miss within the parallel test. TIL I should write more tests :-) another pr coming