Modify IODataOutputInStreamErrorListener interface to support asynchronous notifications when an in-stream error is to be written

[gathogojr]

Issues

This pull request is in partial fulfilment of issue #2019.

Description

Modify IODataOutputInStreamErrorListener interface to support asynchronous notifications when an in-stream error is to be written

Background

When an error occurs when writing a payload to the stream, a notification is sent to any subscribed listener to notify it that an in-stream error is to be written. The listener implements the IODataOutputInStreamErrorListener interface that contains a single synchronous method OnInStreamError. This interface is internal.
To support similar functionality in asynchronous scenarios, we need to modify the interface to support an asynchronous equivalent method - OnInStreamErrorAsync.

ODataBatchWriter is a public abstract class that implements the IODataOutputInStreamErrorListener interface. If OnInStreamErrorAsync method was implemented abstractly in the classes, subclasses of ODataBatchWriter that users of the library might have implemented would break. For that reason, the method has been implemented as a virtual method that just throws a NotImplementedException to prevent the breaking change.
As for other internal classes that implement the interface, an explicit implementation has been added. For now, it just throws a NotImplementedException. The actual implemententation will be provided when implementing asynchronous support in the following 6 sets of writer classes:

• ODataWriterCore/ODataJsonLightWriter
• ODataParameterWriterCore/ODataJsonLightParameterWriter
• ODataCollectionWriterCore/ODataJsonLightCollectionWriter (PR Implement asynchronous support in ODataJsonLightCollectionWriter #2059)
• ODataDeltaWriter/ODataJsonLightDeltaWriter
• ODataBatchWriter/ODataJsonLightBatchWriter
• ODataAsynchronousWriter

Below is how that implementation looks like in ODataCollectionWriterCore. Notice the call to the asynchronous method StartPayloadInStartStateAsync that sets the stage for writing the error.

async Task IODataOutputInStreamErrorListener.OnInStreamErrorAsync()
{
    this.VerifyNotDisposed();

    // We're in a completed state trying to write an error
    // We can't write error after the payload was finished as it might introduce another top-level element in XML
    if (this.State == CollectionWriterState.Completed)
    {
        throw new ODataException(Strings.ODataWriterCore_InvalidTransitionFromCompleted(this.State.ToString(), CollectionWriterState.Error.ToString()));
    }

    await this.StartPayloadInStartStateAsync()
        .ConfigureAwait(false);
    this.EnterScope(CollectionWriterState.Error, this.scopes.Peek().Item);
}

Checklist (Uncheck if it is not completed)

• Test cases added
• Build and test with one-click build and test script passed

Additional work necessary

If documentation update is needed, please add "Docs Needed" label to the issue and provide details about the required document change in the issue.

[habbes]

I wonder if wrapping OnInStreamError in a task would be a more "expected" default implementation for OnInStreamErrorAsync. Is that something you considered? If so, I would be curious to know what dissuaded you from that approach.

Also does the method body prevent breaking change in subclasses that provide the implementation or in those that did not provide the implementation?

[gathogojr]

@habbes Dealing with asynchronous wrappers over synchronous methods is actually what this "true async" initiative is all about so I don't think its a good idea to wrap OnInStreamError.
With regard to your second point, the method body prevents a breaking change in the event that someone had subclassed ODataBatchWriter. The only implementation they would have provided is for the synchronous method and we are not tinkering with that.

[habbes]

@gathogojr on the first point: You make a good point. But I still wonder which default is the lesser evil. My concern is whether there cases where OnInStreamErrorAsync is called without the knowledge or control of the developer who implemented the abstract class. I think this is usually quite likely. If this is the case, then merely updating the library without any additional changes to their program code might lead to unexpected exceptions occurring (possibly occurring in production and undetected in development). This would be quite disruptive, and a breaking change. On the other hand, if the default implementation wraps the OnInStreamError method, then program will retain its current behaviour even after updating the lib. While this implementation is not necessarily ideal for async, it's less disruptive and not a breaking change. If you look at the case of the built-in Stream abstract class, Flush is a abstract and FlushAsync is a virtual method which defaults to wrapping Flush in a task: https://source.dot.net/#System.Private.CoreLib/Stream.cs,189

[habbes]

If we stick with the default of throwing an exception, then I think we should have a way to ensure that developers updating the library in their programs (which could also be libraries used by others) are aware that they need to implement that method or to ensure that that method is not inadvertently called internally by ODL.

[gathogojr]

@habbes I think you misunderstood. The OnStreamInErrorAsync is not triggered automatically, just like the current OnStreamInError is not triggered automatically. Just adding the method does not cause it to be triggered.
Here is how the existing method is currently triggered.
Which is why throwing a NotImplementedException does no harm. FYI, most of the public virtual methods in the public abstract ODataWriter actually throw NotImplementedException. If as a developer you choose to provide an implementation, you'll need to implement the methods that you intend to trigger. It's no different here.

[gathogojr]

If a developer updates the library in their programs, the method will just sit there doing nothing. It'll not be triggered at any point.

[habbes]

@gathogojr I'm a bit confused. Are those methods triggered by the developer's code or by ODL code?
In the link you provided, the OnStreamInError method seems to be explicitly called by ODL code, so that leaves me a bit confused by what you mean when you say "just like the current OnStreamInError is not triggered automatically. In the PR changes, I can't see an instance where OnStreamInErrorAsync is called, who is responsible for calling it and where? If it's never called directly by ODL, then what's the point of having that method on the class? I feel like I'm missing something.

[gathogojr]

@habbes I'll rephrase. The breaking change concern stems from the scenario where a developer has implemented the ODataBatchWriter abstract class. Let's say they call it DeveloperODataBatchWriter. What I meant by saying it is not triggered automatically is that, ODL wouldn't trigger DeveloperODataBatchWriter.OnStreamInErrorAsync at any time. The link I shared is to ODL internal ODataJsonLightOutputContext triggering the method. What I could check is if this internal class can resolve ODataBatchWriter from the DI container before triggering the method because that would be the only way it'd be a breaking change.

[gathogojr]

The other concern I have with your suggestion is that the synchronous and asynchronous approach are using 2 different writers. While they share the same output stream, we verify every step of the way whether the OutputContext instance if set for synchronous or asynchronous writing

[habbes]

@gathogojr and I had an offline chat around this topic and he clarified why this is not a breaking change. Thought I'd share that here as well.

The ODL calls OnInStreamError for the internal implementations of the OutputContext class. This internal implementations only uses internal subclasses of ODataBatchWriter. At no point does it use an ODataBatchWriter that would be passed in by third-party code or via external-facing dependency-injection. So if the user creates their own subclass of the ODataBatchWriter, then we can guarantee that that version will not be called by the internal ODL methods. The only exception is if the user also creates their own OutputContext class, but if they do this, then they are responsible for all the logic, including error handling, whether or not to use error listeners, how to use invoke error listeners, etc. So they will cater for all that (note that a lot of the built-in classes, as well as the IODataOutputInStreamErrorListener interface are internal and not accessible to the developer).

A follow-up question I had was why it was necessary to add the OnStreamInErrorAsync method if it's never called by ODL. The reason it was necessary is because ODataBatchWriter implements IODataOutputInStreamErrorListener and this method has been added to that interface, so either ODataBatchWriter or its concrete subclasses need to implement it. So a basic implementation is added to ODataBatchWriter so that subclasses aren't required to.

[habbes]

Out of curiosity, why is this method explicitly defined for the IODataOutputInStreamErrorListener interface? I realize that this is also the case for the pre-existing OnInStreamError methods, but just curious to know why. Does this class implement multiple interfaces each defining OnInStreamError?

[gathogojr]

Does this class implement multiple interfaces each defining OnInStreamError?

No it doesn't

why is this method explicitly defined for the IODataOutputInStreamErrorListener interface?

I'm not 100% sure why explicit interface implementation route was taken

[gathogojr]

I'm guessing the benefit is that any class that implements ODataBatchWriter abstract class for example can override the default IODataOutputInStreamErrorListener methods either as a public method, or as an explicit interface implementation.

[gathogojr]

@habbes A person who implements ODataBatchWriter abstract class would only bring it into play by implementing the public abstract ODataOutputContext CreateODataBatchWriter method just like we do in ODataJsonLightOutputContext.
By doing that they take the internal ODataJsonLightOutputContext out of play and take the responsibility of triggering the IODataOutputInStreamErrorListener methods.
ODataJsonLightOutputContext has a hard dependency on ODataJsonLightBatchWriter as you can see here

private ODataBatchWriter CreateODataBatchWriterImplementation()
{
    ODataBatchWriter batchWriter = new ODataJsonLightBatchWriter(this);
    this.outputInStreamErrorListener = batchWriter;
    return batchWriter;
}

[pull-request-quantifier-deprecated]

This PR has 16 quantified lines of changes. In general, a change size of upto 200 lines is ideal for the best PR experience!

---

Quantification details

Label      : Extra Small
Size       : +16 -0
Percentile : 6.4%

Total files changed: 7

Change summary by file extension:
.cs : +16 -0

Change counts above are quantified counts, based on the PullRequestQuantifier customizations.

Why proper sizing of changes matters

Optimal pull request sizes drive a better predictable PR flow as they strike a
balance between between PR complexity and PR review overhead. PRs within the
optimal size (typical small, or medium sized PRs) mean:

• Fast and predictable releases to production:
  • Optimal size changes are more likely to be reviewed faster with fewer
    iterations.
  • Similarity in low PR complexity drives similar review times.
• Review quality is likely higher as complexity is lower:
  • Bugs are more likely to be detected.
  • Code inconsistencies are more likely to be detetcted.
• Knowledge sharing is improved within the participants:
  • Small portions can be assimilated better.
• Better engineering practices are exercised:
  • Solving big problems by dividing them in well contained, smaller problems.
  • Exercising separation of concerns within the code changes.

What can I do to optimize my changes

• Use the PullRequestQuantifier to quantify your PR accurately
  • Create a context profile for your repo using the context generator
  • Exclude files that are not necessary to be reviewed or do not increase the review complexity. Example: Autogenerated code, docs, project IDE setting files, binaries, etc. Check out the Excluded section from your prquantifier.yaml context profile.
  • Understand your typical change complexity, drive towards the desired complexity by adjusting the label mapping in your prquantifier.yaml context profile.
  • Only use the labels that matter to you, see context specification to customize your prquantifier.yaml context profile.
• Change your engineering behaviors
  • For PRs that fall outside of the desired spectrum, review the details and check if:
    • Your PR could be split in smaller, self-contained PRs instead
    • Your PR only solves one particular issue. (For example, don't refactor and code new features in the same PR).

How to interpret the change counts in git diff output

• One line was added: +1 -0
• One line was deleted: +0 -1
• One line was modified: +1 -1 (git diff doesn't know about modified, it will
  interpret that line like one addition plus one deletion)
• Change percentiles: Change characteristics (addition, deletion, modification)
  of this PR in relation to all other PRs within the repository.

---

Was this comment helpful? 👍  :ok_hand:  :thumbsdown: (Email)
Customize PullRequestQuantifier for this repository.