Handle exceptions and thread interrupts during DocumentContext lock

[JerryShea]

The below code implements a typical pattern to write to a chronicle i.e. get the writing DocumentContext in a try-with-resources and close it when finished writing.

public class BinaryMethodWriterInvocationHandler ...
    ...
    @Override
    protected void handleInvoke(Method method, Object[] args) {
        try (@NotNull DocumentContext context = appender.writingDocument()) {
            Wire wire = context.wire();
            handleInvoke(method, args, wire);
            wire.padToCacheAlign();
        }
    }

If our thread is interrupted during the serialisation (which in this case happens during handleInvoke) then a java.nio.channels.ClosedByInterruptException is thrown during the close of the context. As the close seems to perform most of its function still, it appears to the chronicle (and any readers) that a valid object has been written although the object is only partially written.

See net.openhft.chronicle.queue.impl.single.NotCompleteTest#testInterruptedDuringSerialisation

[peter-lawrey]

We could have a common pattern for rolling back a message if an exception is thrown.

…

On 24 Oct. 2017 06:30, "Jerry Shea" ***@***.***> wrote: The below code implements a typical pattern to write to a chronicle i.e. get the writing DocumentContext in a try-with-resources and close it when finished writing. public class BinaryMethodWriterInvocationHandler ... ... @OverRide protected void handleInvoke(Method method, Object[] args) { try ***@***.*** DocumentContext context = appender.writingDocument()) { Wire wire = context.wire(); handleInvoke(method, args, wire); wire.padToCacheAlign(); } } If our thread is interrupted during the serialisation (which in this case happens during handleInvoke) then a java.nio.channels.ClosedByInterruptException is thrown during the close of the context. As the close seems to perform most of its function still, it appears to the chronicle (and any readers) that a valid object has been written although the object is only partially written. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#390>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABBU8UMiRdohiC2rVaCgeud6Og7heCuFks5svXX3gaJpZM4QD3dn> .

[JerryShea]

Yes - agree. Although with try-with-resources the catch is run after the resource is closed. I think we need something like

        @NotNull DocumentContext context = appender.writingDocument();
        try {
            Wire wire = context.wire();
            handleInvoke(method, args, wire);
            wire.padToCacheAlign();
        } catch (Throwable t) {
            context.setRollback();
        } finally {
            context.close();
        }

which unfortunately is not as concise

[peter-lawrey]

For the most part, we should offer a higher level interface which means this try/catch/finally block shouldn't need to be written by end users (or not often) ᐧ

…

On 24 October 2017 at 09:04, Jerry Shea ***@***.***> wrote: Yes - agree. Although with try-with-resources the catch is run after the resource is closed. I think we need something like @NotNull DocumentContext context = appender.writingDocument(); try { Wire wire = context.wire(); handleInvoke(method, args, wire); wire.padToCacheAlign(); } catch (Throwable t) { context.setRollback(); } finally { context.close(); } which unfortunately is not as concise — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#390 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABBU8S320m-oKC7ZKlMhvWxf22ibnVU1ks5svZoggaJpZM4QD3dn> .

[RobAustin]

@JerryShea I'm keen not to have a list of git issue that we never address, can you work on this or close is as a point of discussion.

[JerryShea]

@RobAustin, there is still a problem when we detect this situation and recover from it, but only with lazy indexing. I have a test for it, just not a fix yet. Will work on this week. Now fixed with commit fa47af7
@peter-lawrey, agree with you about the higher level interface. Instead of the current pattern

        try (@NotNull DocumentContext context = appender.writingDocument()) {
           //  do something with context;
        }

I suppose we should support an API something like this

appender.writingDocument().writeWithContext(context -> {
           //  do something with context;
});

Any comments?

[peter-lawrey]

The problem with lambdas is structuring them so they don't create garbage. There are tricks to make them non capturing but it would be useful to see some real examples.

…

On 4 Dec. 2017 21:28, "Jerry Shea" ***@***.***> wrote: @RobAustin <https://github.com/robaustin>, there is still a problem when we detect this situation and recover from it, but only with lazy indexing. I have a test for it, just not a fix yet. Will work on this week. @peter-lawrey <https://github.com/peter-lawrey>, agree with you about the higher level interface. Instead of the current pattern try ***@***.*** DocumentContext context = appender.writingDocument()) { // do something with context; } I suppose we should support an API something like this appender.writingDocument().writeWithContext(context -> { // do something with context; }); Any comments? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#390 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABBU8ThGEeA427hFvVx9a3fSLTpS6Nsfks5s9GP6gaJpZM4QD3dn> .

[JerryShea]

So, BinaryMethodWriterInvocationHandler could look something like this:

appender.writingDocument().writeWithContext(context -> {
    Wire wire = context.wire();
    handleInvoke(method, args, wire);
    wire.padToCacheAlign();
});

But the lambda may well have to moved in to a field as it captures this.handleInvoke in the above example.
The writeWithContext method would do the following:

void writeWithContext(Consumer<DocumentContext> toRun) {
    try {
        toRun.apply(context);
    } catch (Throwable t) {
        context.setRollback();
        Jvm.rethrow(t);
    } finally {
        context.close();
    }
}

And so any exception that occurred during writing would mean that the context would rollback the write. Some exceptions may mean that it is not possible to rollback the write (e.g. if we can no longer write to the chronicle) and so in this case, timedStoreRecovery will have to kick in next time it read from or written to.

[peter-lawrey]

Can you include the full non gc code as you may find this is more complicated than having a rollback.

…

On 4 Dec. 2017 23:45, "Jerry Shea" ***@***.***> wrote: So, BinaryMethodWriterInvocationHandler could look something like this: appender.writingDocument().writeWithContext(context -> { Wire wire = context.wire(); handleInvoke(method, args, wire); wire.padToCacheAlign(); }); But the lambda may well have to moved in to a field as it captures this.handleInvoke in the above example. The writeWithContext method would do the following: void writeWithContext(Consumer<DocumentContext> toRun) { try { toRun.apply(context); } catch (Throwable t) { context.setRollback(); Jvm.rethrow(t); } finally { context.close(); } } And so any exception that occurred during writing would mean that the context would rollback the write. Some exceptions may mean that it is not possible to rollback the write (e.g. if we can no longer write to the chronicle) and so in this case, timedStoreRecovery will have to kick in next time it read from or written to. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#390 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABBU8W7IgdGiYMYrZpXWYLCYzJOHdqNwks5s9IQMgaJpZM4QD3dn> .

[peter-lawrey]

We could have an option to the context where a success method must be called or the message is rolled back. On 5 Dec. 2017 00:16, "Peter Lawrey" <peter.lawrey@chronicle.software> wrote:

…

Can you include the full non gc code as you may find this is more complicated than having a rollback. On 4 Dec. 2017 23:45, "Jerry Shea" ***@***.***> wrote: > So, BinaryMethodWriterInvocationHandler could look something like this: > > appender.writingDocument().writeWithContext(context -> { > Wire wire = context.wire(); > handleInvoke(method, args, wire); > wire.padToCacheAlign(); > }); > > But the lambda may well have to moved in to a field as it captures > this.handleInvoke in the above example. > The writeWithContext method would do the following: > > void writeWithContext(Consumer<DocumentContext> toRun) { > try { > toRun.apply(context); > } catch (Throwable t) { > context.setRollback(); > Jvm.rethrow(t); > } finally { > context.close(); > } > } > > And so any exception that occurred during writing would mean that the > context would rollback the write. Some exceptions may mean that it is not > possible to rollback the write (e.g. if we can no longer write to the > chronicle) and so in this case, timedStoreRecovery will have to kick in > next time it read from or written to. > > — > You are receiving this because you were mentioned. > Reply to this email directly, view it on GitHub > <#390 (comment)>, > or mute the thread > <https://github.com/notifications/unsubscribe-auth/ABBU8W7IgdGiYMYrZpXWYLCYzJOHdqNwks5s9IQMgaJpZM4QD3dn> > . >

[JerryShea]

So we have 3 options:

1. higher level interface so the user doesn't have to worry about try/catch/finally. This is a bit of effort and a bit of change to the API
2. add a setRollback method to DocumentContext so the user can use the try/catch/finally pattern
3. add an option to context where a success (commit?) method must be called otherwise rollback

The easiest and lowest risk of these is 2. so I have implemented that with OpenHFT/Chronicle-Wire@0ffaeb1

[RobAustin]

@JerryShea. Looks good, suggestion - rename setRollback() to rollbackWhenClosed() or perhaps just rollback()

…

On 7 Dec 2017, at 6:25 am, Jerry Shea ***@***.***> wrote: So we have 3 options: higher level interface so the user doesn't have to worry about try/catch/finally. This is a bit of effort and a bit of change to the API add a setRollback method to DocumentContext so the user can use the try/catch/finally pattern add an option to context where a success (commit?) method must be called otherwise rollback The easiest and lowest risk of these is 2. so I have implemented that with ***@***.*** — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

[peter-lawrey]

Note: we have the MethodReader / Writer as a higher level interface and this should use any solution we come up with.

…

On 7 Dec. 2017 06:25, "Jerry Shea" ***@***.***> wrote: So we have 3 options: 1. higher level interface so the user doesn't have to worry about try/catch/finally. This is a bit of effort and a bit of change to the API 2. add a setRollback method to DocumentContext so the user can use the try/catch/finally pattern 3. add an option to context where a success (commit?) method must be called otherwise rollback The easiest and lowest risk of these is 2. so I have implemented that with ***@***.*** <OpenHFT/Chronicle-Wire@0ffaeb1> — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#390 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABBU8bfckeNorQQnPdX9SSH1WrnTeIuBks5s94TYgaJpZM4QD3dn> .

[JerryShea]

@peter-lawrey - yes - see OpenHFT/Chronicle-Wire@6ece62b

[JerryShea]

Have documented this behaviour in a test and added doco.