ContentSourcePublisher throws from request

[gregw]

Jetty version(s)

Jetty Environment 12

Description

In the spring integration branch we are seeing the following exception:

java.lang.IllegalStateException: channel already completed
	at org.eclipse.jetty.server.internal.HttpChannelState$ChannelRequest.lockedGetHttpChannelState(HttpChannelState.java:809) ~[jetty-server-12.0.6.jar:12.0.6]
	at org.eclipse.jetty.server.internal.HttpChannelState$ChannelRequest.read(HttpChannelState.java:873) ~[jetty-server-12.0.6.jar:12.0.6]
	at org.eclipse.jetty.io.content.ContentSourcePublisher$SubscriptionImpl.process(ContentSourcePublisher.java:114) ~[jetty-io-12.0.6.jar:12.0.6]
	at org.eclipse.jetty.io.content.ContentSourcePublisher$SubscriptionImpl.request(ContentSourcePublisher.java:85) ~[jetty-io-12.0.6.jar:12.0.6]
	at org.reactivestreams.FlowAdapters$ReactiveToFlowSubscription.request(FlowAdapters.java:182) ~[reactive-streams-1.0.4.jar:?]
	at reactor.core.publisher.FluxMap$MapSubscriber.request(FluxMap.java:164) ~[reactor-core-3.6.2.jar:3.6.2]
	at reactor.core.publisher.BaseSubscriber.request(BaseSubscriber.java:214) ~[reactor-core-3.6.2.jar:3.6.2]
	at org.springframework.http.codec.multipart.MultipartParser.requestBuffer(MultipartParser.java:196) ~[spring-web-6.1.4-SNAPSHOT.jar:6.1.4-SNAPSHOT]

However, the reactive specification says that "Calling Subscription.request MUST return normally". So our ContentSourcePublisher is breaking that rule and throwing rather than calling onError

[gregw]

@sbordet do you agree this is a bug? @lachlan-roberts I'm not sure if this is causing some spring-framework integration failures, but it is certainly not helping us fail in a normal way.

[gregw]

Actually @lorban do you think this is a violation of our Content.Source#read() contract? Should it return an error chunk rather than throw?

[sbordet]

We throw for "must not happen" kind of situations.

Returning an error chunk is more like a failure that may happen (e.g. early EOF).

I agree that we should protect a call to request(), but the fact that we have a visible stack trace is actually a plus, rather than maybe a warning log that may not be seen.

Why there is a call to request() with the channel already completed?

[joakime]

Interesting discussion back in 2014 about throwing from the reactive Subscription APIs. https://github.com/reactive-streams/reactive-streams-jvm/issues/52

We throw for "must not happen" kind of situations.

When I first read this, it felt like you were describing a fatal error.

To the reactive folks back in 2014 a fatal error is only something that will have a high likelyhood of shutting down the JVM. (think OOM, VMError, etc) But then a bunch of examples of fatal (but not JVM failures) were presented in the above discussion, but it didn't sway the group to allow throwing from any Subscription API. Basically, while the discussion was focused on specifically onError() the behaviors of the rest of the API and how the API is used pretty much says we cannot throw, even in the "must not happen" types, and must use onError to report that.

[lorban]

IMHO Content.Source#read() should never throw for at least two reasons:

• Consistency with methods taking a callback: they always fail the callback and never throw.
• Clear out uncertainty: there's only one way to be notified of errors, so you don't have to wear a belt (isError()) & suspenders (try / catch).

[gregw]

We throw for "must not happen" kind of situations.

Returning an error chunk is more like a failure that may happen (e.g. early EOF).

I agree that we should protect a call to request(), but the fact that we have a visible stack trace is actually a plus, rather than maybe a warning log that may not be seen.

Why there is a call to request() with the channel already completed?

@sbordet I've not yet looked into the cause of the late call to read, but it is from a unit test and for all we know it could be testing error handling. I.e. what happens if the connection closes early.

I was looking for a normal termination of the Flux, but didn't see one.. but thought that we should say least see an onError

I'd expect the stack trace would still be visible if it was reported to onError and was not expected.

I'll prepare a branch with this fixed and see how that affects the spring test... Probably won't fix it, but we'll see.

[gregw]

• Clear out uncertainty: there's only one way to be notified of errors, so you don't have to wear a belt (isError()) & suspenders (try / catch).

and if you only catch Exception you can still end up with your pants around your ankles if somebody throws a Runtime!

I'll fix