Increase robustness of outbox flushing with respect to outbox entry deserialization issues

[skastenholz]

Background:

• We are utilizing this framework in all our microservices for certain requests to external and internal downstream services.
• In doing so JacksonInvocationSerializer is used to serialize/deserialize invocations (in a most flexible way).

Problem:

• On rare occassions some outbox entries that were serialized cannot be deserialized successfully.
• For instance this happens when the id of multiple attributes are set to null. However, the concrete issue is not relevant in this context. Most importantly the the call of "deserializeInvocation" at JacksonInvocationSerializer (https://github.com/gruelbox/transaction-outbox/blob/master/transactionoutbox-jackson/src/main/java/com/gruelbox/transactionoutbox/jackson/JacksonInvocationSerializer.java#L46) fails with an IOException.

Analysis:

• As a result the IOException is wrapped in a RuntimeException and is populated to
  • https://github.com/gruelbox/transaction-outbox/blob/master/transactionoutbox-core/src/main/java/com/gruelbox/transactionoutbox/TransactionOutboxImpl.java#L129C53-L129C64
  • https://github.com/gruelbox/transaction-outbox/blob/master/transactionoutbox-core/src/main/java/com/gruelbox/transactionoutbox/TransactionOutboxImpl.java#L142
• Ultimately, the processing of outbox entries stops entirely - even in case the problems applies to a single outbox entry only as part of the batch. As a result no outbox entries are processed.
• In addition the same happens when the outbox is flushed again at a later point in time.
• Finally, all outbox entries of the microservice are no longer processed.
• A similar observation was made in Versioning on invocations serialized to the database #628.

Solution:

• The approach suggested here is to handle the IOException at a later point in time at the DefaultPersistor.
• During deserialization of all relevant outbox entries the ones on which the problems applied are ignored (instead of failing the whole operation).
• In doing so deserializeable entries are processed and the processing of remaining outbox entries is not blocked.

Open points:

• Please review the solution and provide your feedback.
• With respect to tests I would plan to extend AbstractPersistorTest, but am struggling since the InvocationSerializer is not exposed yet. Any hints would be welcome.

[badgerwithagun]

This looks like a sensible first step. I think, however, that we should follow normal error behaviour for anything that fails to deserialise:

• push back the nextAttemptTime
• increment the attemptCount (if the record has no group, i.e. is unordered)
• Fire the appropriate TransactionOutboxListener events if appropriate (with a syntheticTransactionOutboxEntry constructed from the normal fields on the record but with a dummy invocation)

This will avoid the same record getting re-read continously, allow blocking to progress as normal, and ensure that anyone who is using TransactionOutboxListener to monitor things to detect that there is a problem.

[badgerwithagun]

We're also going to need testts for these behaviours!

[skastenholz]

Alright, good point. I tried different approaches and came up with the one comitted. In doing so I needed to modify core classes and, in particular, you may have suggestions regarding the Invocation class. Also I added/modified some tests.

Please have a look and provide your feedback. Thanks.

[badgerwithagun]

Rather than adding a new property here (which will impact on ser/deser of all Invocations, not just this one), why not create a FailedDeserializingInvocation, something like this:

class FailedDeserializingInvocation extends Invocation {

  private final IOException e;

  FailedDeserializingInvocation(IOException e) {
    this.className = "";
    this.methodName = "";
    this.parameterTypes = new Class<?>[] {};
    this.args = new Object[] {};
    this.mdc = null;
    this.e= e;
  }

  @Override
  void invoke(Object instance, TransactionOutboxListener listener)
      throws NoSuchMethodException, IllegalAccessException, InvocationTargetException {
     throw new WrappedCheckedException(e);
  }

}

Should make the code elsewhere cleaner too.

You will need to replace @Value on Invocation to make it nonfinal so this can be done:

@ToString
@EqualsAndHashCode
@AllArgsConstructor
@FieldDefaults(makeFinal = true, level = AccessLevel.PRIVATE)
@Getter

[skastenholz]

Yes, makes sense. Done.

[badgerwithagun]

I think there's a standard exception somewhere in the core lib for generically wrapping checked exceptions, which is all that's going on here.

[skastenholz]

Thanks for the hint. Done.

[badgerwithagun]

This is a really nice approach. I like how neatly it works when you push the exception down into the invocation. Great stuff.

Just a few tweaks on the implementation, but the approach and testing are sound.

[badgerwithagun]

Merged! Thanks :)