Experiment/jetty 12 chunk isError and warnings

[gregw]

Experiment to see:

• how Chunk works without a known Error type and instead use an isError method.
• to try transient errors/warnings in a Content.Source

[gregw]

@lorban I have removed the Content.Chunk.Error type in favour of a Content.Chunk.isError method. I think that bit is pretty good.

I have also started experimenting with transient failures (calling them warnings for now). I've updated AsyncSource and the ContentSourceInputStream. Both mostly trivial updates, but both definitely needed updates. So on that sample, every Source and many Source.Consumers will need at least a little change to deal with warnings.

I've added you as an assignee rather than as a reviewer, so please feel free collaborate and experiment in this branch to see how you like it. I think there needs to be a lot more tests of mods to the basic Sources before we even think of trying to put an idletimeout through as a warning.

[gregw]

@sbordet this is ready for your review.

@lorban and I tossed around the naming for a while. We ended up with:

• All chunks have a getError() method, which can return null
• There is a static boolean Chunk.isError(Chunk) to make it easy to do a null chunk and null error check
• We use the term "failure" for errors that have last==true
• We use the term "warning" for errors that have last=false

We have updated a few things to handle "warnings":

• AsyncContent
• Content.copy methods
• ContentSourceInputStream
• ContentDocs

Our feeling is that this change is good in that it removes the Error type and that failure vs warning is pretty clear. However, almost every caller of Source.read() will need careful review about how it handles errors.

Currently nothing we have will inject a warning into any of our sources. Ultimately addIdleTimeoutListener could be changed so that a false return from the listener means use a warning rather than a failure.... or it could be try state?

[lorban]

@sbordet here is the gist of this API modification, other changes are about adapting the implementation:

• Error checking is done with Chunk.isError(), not anymore with instanceof Chunk.Error, and the latter class got removed from the exposed API.
• Chunk now has a Throwable getError() method:
  • Error chunks must return non-null from getError().
  • Non-error chunks must return null from getError().
• Error chunks aren't necessarily last anymore:
  • When they are last, they are persistent like they used to be. Those are now referred to as failures.
  • When they are not last, the following read and demand calls may return non-error data. Those are now referred to as warnings.
• Source.warn(Throwable) has been added next to Source.fail(Throwable).
• Error chunks cannot be retainable and must be backed by an empty byte buffer.

[sbordet]

@gregw the only think I am worried is a spin loop in case of transient failed Chunks.
Do we have a test case for that?
And is the semantic that once the transient failed Chunk has been read, another read returns null (or something else)?

[sbordet]

Rename to isFailure().

[gregw]

@sbordet The names were discussed at length between @lorban and myself. We did try the renames that you suggest, but they don't really work.

Currently we have isError and getError, where an "error" can be a failure from fail(Throwable) or a warning from warn(Throwable)

An alternative is to have isFailure and getFailure, where a "failure" can be a persistent from fail(Throwable) or a either from fail(Throwable failure, boolean last). @lorban thoughts?

Eitherway, the language used is more than just these 2 methods and updates need to be coordinated through the javadoc and other methods.

[gregw]

@sbordet note that @lorban and I did consider the fail(Throwable failure, boolean last) approach, but there are big differences between the semantics of last being true/false.
With last==true, any queued chunks are failed and the source is drained and failed immediately. With last==false, the "warning" is handled in order with other chunks and no draining occurs.

I guess that can be OK behind the one method with careful javadoc?

[lorban]

I vote -0 on fail(Throwable failure, boolean last) for the reasons @gregw explained: the last flag completely changes the semantic so I'd prefer to have two separate methods, but could live with a single well-documented one.

Error, failure and warning have very specific meanings in this context, so maybe they should be better documented but those three words were carefully chosen.

[gregw]

I'm also -0 on the single method. Having said that, I'm also only +0 on the warn(Throwable) name.
It is interesting that to describe what a warning is, I tend to say it is a "transient failure" rather than a "transient error".

So I could definitely be +1 on isFailure(Chunk) and getFailure() if we could work out a good way of injecting a transient failure. That way we could talk about "failures" only and their attributes (last or not last), and not have to distinguish between "warning" and "error", as they would not apply.

Perhaps fail(Throwable) and transientFail(Throwable)?

Or maybe fail(Throwable failure, boolean last) is not so bad if we javadoc it really well?

[gregw]

@lorban thoughts on naming? again?

[lorban]

I prefer to stick to what we have: isError, getError and fail(Throwable). I just wouldn't add warn(Throwable) on Content.Source yet and only have it on AsyncContent for now.

[gregw]

@lorban now you say it... I'm liking less the usage of error, fail and warn, when we could just do fail and javadoc. so I'm ±0 for the current naming. @sbordet if you do look at this, what are your thoughts now we've explained the dilema?

[sbordet]

I would stick with fail(Throwable), Throwable getFailure() and isFailed(Chunk).

[gregw]

@gregw the only think I am worried is a spin loop in case of transient failed Chunks. Do we have a test case for that?

Note really. Since transient failures (aka warnings) are transient, then we'd only loop if something kept raising them over and over again. Currently we have no mechanism to introduce warnings (aka transient failures) other than in test harnesses.... but if we were, it would be as a result of an onIdleTimeout listener callback. The listener would have to keep saying "treat this as a transient timeout" over and over in order to loop.

The related question, for when we want to actually use these transient failures is should onIdleTimeout be bi-state (timeout is permanent or transient) or tri-state (permanent, transient, ignored). Currently we have (permanent or ignored), so it we just went bi-state, we'd loose the ability to totally ignore a timeout... but I think this is OK.

However, we need to review all our callers of read() before we contemplate actually using this mechanism. This PR looked at a few and each needed small changes, so probably all will need it. I'm happy to visit all readers... but we need to get the language (transient failure vs warning) sorted out before I do.

And is the semantic that once the transient failed Chunk has been read, another read returns null (or something else)?

It returns null. See Content.Chunk.next(Chunk). It is just a chunk to be consumed like any other non-last chunk.

[gregw]

@lorban can I get a review on this modulo naming?

[lorban]

After pondering this change, I'm now wondering if isError() shouldn't do return chunk != null && chunk.getError() != null && chunk.isLast().

All the existing code that used to do chunk instanceof Error would then do the right thing both in case of a failure and a warning without any other change than isError(chunk): failures would mark the end of the source, and warnings would be considered like a normal content with a zero-byte buffer; this would all be safer by default.

[gregw]

After pondering this change, I'm now wondering if isError() shouldn't do return chunk != null && chunk.getError() != null && chunk.isLast().

That could be an isFailure(Chunk) method, but it would be confusing to have an isError(Chunk) that returns false when getError() is non-null. May be it should be getCause() again?

All in all, I think all code needs to be revisited to make sure it can handle an non-last error/failure/warning, so I don't think this variation is necessary.

[gregw]

@lorban The comment on the call last night regarding a Throwable that is an "error" but not a java.lang.Error has convinced me that just using "failure" is the better option. I'll prepare a variant like that to see.

[gregw]

@lorban in preparing the rename I found lots of javadoc that linked through to java.lang.Error, convincing me even more.

[gregw]

@lorban I have pushed the rename back to isFailure and getFailure.

I think the distinction between "error", "failure" and "warning" would have been fine if all we were talking about was Chunks, but we are in an environment with java.lang.Error and 'Callback.failed(Throwable)andXyzListener.onError(Throwable), so that any naming purity we established withing a chunk was lost the moment we used that with other code, where we immediately saw things like callback.failed(chunk.getError()). We now still have similar things like listener.onError(chunk.getFailure())`, but we are no longer trying to make a semantic difference based on "failure" vs "error" vs "warning", so such code is less confusing.

So I'm now a definitely +1 on the just call it "failure" camp. Hopefully I've done a reasonable job in the javadoc to explain transient vs persistent failures? I have also added a Chunk.isFailure(Chunk chunk, boolean last) method so you can easily check on the type of failure if you want (many tests do).

on vacation. review feedback met.

[lorban]

Just a few small things left.

Overall, I'm happy with this change and you're right about the purity of error vs failure vs warning that doesn't make sense in the overall picture. I'm still convinced isFailure(chunk, true) is the right thing to do by default to check for errors, but we can re-review all that later and we don't generate transient failures for now so what we have now currently is safe. And I do agree that isFailure(chunk) should not check the last flag. The contract totally LGTM.

[lorban]

One last nit.

[sbordet]

This method is never called with last=false?
Also, I don't think it is necessary, as if isFailed(Chunk)==true then you can safely call chunk.isLast() without NPE.

[gregw]

It is used in lots of places, albeit mostly tests. No harm having it.

[sbordet]

I would stick with fail(Throwable), Throwable getFailure() and isFailed(Chunk).

[gregw]

@lorban can I get another review, I lost the green tick fixing some niggles from @sbordet's review