[Binance] Orderbook initial snapshot

[badgerwithagun]

Resolves #86. Explanation given on there.

[davidjirovec]

I am afraid there is some memory leak. When I modify manual example like following

public static void main(String[] args) throws InterruptedException {
    final ExchangeSpecification exchangeSpecification = new ExchangeSpecification(BinanceStreamingExchange.class);
    exchangeSpecification.setShouldLoadRemoteMetaData(true);
    StreamingExchange exchange = StreamingExchangeFactory.INSTANCE.createExchange(exchangeSpecification);
    ProductSubscription subscription = exchange.getExchangeSymbols().stream().limit(50).reduce(ProductSubscription.create(), ProductSubscription.ProductSubscriptionBuilder::addOrderbook, (productSubscriptionBuilder, productSubscriptionBuilder2) -> {
        throw new UnsupportedOperationException();
    }).build();
    exchange.connect(subscription).blockingAwait();
    Thread.sleep(MAX_VALUE);
}

After 45 minutes memory usage look like this, pattern keeps growing like this even when "Perform GC" is clicked and eventually it would run out of memory.

[badgerwithagun]

@davidjirovec : drat. Thanks for that report. Really useful. Did you confirm that this wasn't occurring beforehand?

I'll look into this in the next couple of days.

[badgerwithagun]

Just checked my production app and it has been quietly cycling every few hours due to OOM errors, but only slightly more often than it always has. There's definitely already a memory leak somewhere. If this has made it worse, that provides some clues perhaps...

[davidjirovec]

I didn't use xchange-stream binance until this PR. You used some previous version even when it gave you OOM? :)

[badgerwithagun]

Background thread issuing a System.exit() every few hours and the container restarts itself.

Filth I know, but I have had more important things to worry about. Memory leaks are somewhere below Urgent.

I will look at this properly now tho.

[davidjirovec]

Ok, I understand :)

[badgerwithagun]

[snipped] - see below

[badgerwithagun]

[snipped] - see below

[badgerwithagun]

Confirmed. This memory leak goes away when #191 is merged.

@davidjirovec , if you'd like to confirm yourself, compare the results of running your test against this branch (binance-orderbook-initial-snapshot) and develop-fork, both on my fork. The only real material differences between the branches are that the latter has some slightly newer dependencies (but I've tested and they aren't having any effect) and that the latter contains #191, which cleans up connection/reconnection issues.

binance-orderbook-initial-snapshot:

develop-fork:

In short, I think this PR is OK. It's just (yet) another reason why #191 needs to be merged.

[davidjirovec]

Thanks, I will test it. develop-fork should be handling orderbooks correctly? Because it to me it seems the code is missing. But I've found your branch develop-fork-develop (wow :) ) and to me it seems it contains both fixes for connection issues and orderbook handling? But maybe I am not right, it is complicated :)

[badgerwithagun]

Thanks, I will test it. develop-fork should be handling orderbooks correctly? Because it to me it seems the code is missing. But I've found your branch develop-fork-develop (wow :) ) and to me it seems it contains both fixes for connection issues and orderbook handling? But maybe I am not right, it is complicated :)

• develop-fork contains this fix plus Fix gdax disconnect NPE/Bitmex execution reports/bug-fix rollup #191 (plus a few other things, but I've already tested with and without those changes, and they don't affect this result).
• binance-orderbook-initial-snapshot contains only this fix.

You should find that develop-fork produces a flat memory profile, but binance-orderbook-initial-snapshot shows the leak. This shows (to me at least) that the leak has nothing to do with this PR but is fixed by #191.

[badgerwithagun]

Sorry @davidjirovec, you were quite right, develop-fork was missing this PR. So many forks. I've pushed the update now, but don't run your tests yet. I'm just double-checking. I'm certain I tested this on develop-fork-develop too, but now I'm doubting myself.

[badgerwithagun]

I was indeed confusing myself. I've now replicated this on develop-fork too.

OK, I will go back to trying to work out what the cause is.

[badgerwithagun]

@davidjirovec - found it, fixed

Highlighted is a manual GC.

[davidjirovec]

Tested myself, looks good!

[badgerwithagun]

@davidjirovec, are you able to post an accepting review (see the Review Required section below)? Then we can get it merged.

[davidjirovec]

Says "At least 1 approving review is required by reviewers with write access." :-/

[Flemingjp]

@davidjirovec It just needs someone with write access to also accept - I can sort this out for you

[Flemingjp]

Based off approving review @davidjirovec