Retrieve the value from seenMessages once to avoid NullPointerException

[ajsutton]

In AbstractRouter seenMessages is an LRU map so it automatically removes items when new ones are added to stay below some maximum size. So we could wind up with a message in msgSubscribed which is present in seenMessages when we first create msgUnseen but is then evicted when we we add the unseen messages on the second line. Then when we get to the last line seenMessages[getMessageId(it)] returns null because the message has now been dropped from seenMessages.

To avoid this, retrieve the value from seenMessages once and hold on to it while processing. This avoids the risk that the value is dropped while recording the newly seen messages.

A less invasive change would be to move the line:

msgUnseen.forEach { seenMessages[getMessageId(it)] = Optional.empty() }

down below the line that calls notifySeenMessage which is the second place we look up the value from seenMessages. But that feels like a very subtle dependency on ordering.

fixes Consensys/teku#2452

[Nashatyrev]

Cool! Looks like you are solve the puzzle 👍 From my initial review I though that something could be called on another thread.
Though it looks like a suspicious 'luck' cause seenMessages has a limit of 10000 messages and we are receiving a message which is exactly 10000 messages old. Anyway this fix is a move to the right direction.

And yep, if you are going to use !! in Kotlin you should think twice. Like the class cast in Java, the !! is also a type cast.

[Nashatyrev]

Though I'm just slightly concerned regarding the style like doing other things (especially with side effects) in filter besides of filtering
I would suggest something in a more functional style (avoiding !!) like:

        msgSubscribed
            .mapNotNull { msg -> seenMessages[getMessageId(msg)]?.let { msg to it } }
                // seen message -> validation result
            .forEach { (msg, res) -> notifySeenMessage(peer, msg, res) }

What do you think?

[ajsutton]

You I wasn't all that keen on doing work in the filter either. My first thought was to just use a plain for each loop and add to an array manually. ie:

List msgUnseen = new ArrayList<>();
for (Any msg in msgSubscribed) {
 .....
 msgUnseen.add(msg);
}

but I could easily work out what the Kotlin equivalent of that was so just went with filter. Your suggested change would also work, though it does leave open the possibility that a message will wind up being neither seen nor unseen which is a little odd, but I don't think it actually breaks anything.

I'm happy to go with whatever you think is best.

[Nashatyrev]

Let's stick to your suggested code. It looks clearer imho, despite may be that way of filter() usage

[Nashatyrev]

I would just rename seenMessage to validationResult for clarity