Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RC1 TCK: IdentityProcessorVerification expectations #30

Open
akarnokd opened this issue Mar 9, 2017 · 8 comments
Open

RC1 TCK: IdentityProcessorVerification expectations #30

akarnokd opened this issue Mar 9, 2017 · 8 comments

Comments

@akarnokd
Copy link
Contributor

akarnokd commented Mar 9, 2017

Maybe this isn't the right place for this but currently I have code available for C# (RxJava 2 doesn't do IdentityProcessorVerification).

I've encountered problems with the IdentityProcessorVerification:

Required_spec104_mustCallOnErrorOnAllItsSubscribersIfItEncountersANonRecoverableError

Looking at the source code, this test expects that an Exception is delivered eagerly to an ISubscriber even if it hasn't requested. My fanout IPublisher implementation first delivers available items and then any error which I believe is also legal. Maybe the verification at L347 could first check if there was an error emitted and if not, request 1 from sub2 and then check for the value and the error.

Required_exerciseWhiteboxHappyPath

Looks like this test expects that when a single the ISubscriber cancels its ISubscription, the IProcessor cancels its ISubscription. The same fanout IPublisher won't cancel its ISubscription as ISubscribers are allowed to come and go at will. I think both behaviors are acceptable and maybe this check should be optionally disabled via a public virtual long SupportsUpstreamCancelOnSubscriberCancel { get; } parameter for example.

@akarnokd
Copy link
Contributor Author

akarnokd commented Mar 9, 2017

In addition, I have trouble with Required_mustRequestFromUpstreamForElementsThatHaveBeenRequestedLongAgo:

Reactive.Streams.TCK.IdentityProcessorVerification`1.TestSetup.ExpectNextElement(ManualSubscriber`1 subscriber, T expected)

timeout while awaiting 2 within 25 ms

Without line numbers, I speculate L552 fails due to the lockstep nature of my fanout IProcessor: if items are available but not all ISubscribers are ready to receive, nobody receives an item.

@viktorklang
Copy link
Contributor

onComplete and onError should not require request to propagate downstreams.

@akarnokd
Copy link
Contributor Author

akarnokd commented Apr 2, 2017

@viktorklang If the upstream emission sequence was onNext, onError and the processor is not fail-fast, one needs a request(1) to get the onNext item immediately followed by onError.

@viktorklang
Copy link
Contributor

@akarnokd If the error has been detected/encountered by the upstream it should be propagated to current subscribers.

@akarnokd
Copy link
Contributor Author

akarnokd commented Apr 3, 2017

In practice, many stream users want to process as many items as received and delay the error to the very end. This is why most operators that have an implicit or explicit async boundary have the option to have the onError cut ahead (fail-fast mode) or not (delay-error mode). Just because Akka-Stream wasn't designed in this mind it doesn't mean other Reactive-Streams implementation are forbidden to do so and also pass the TCK in delay-error mode.

§1.4: If a Publisher fails it MUST signal an onError.

Doesn't say or require an immediate notification.

@viktorklang
Copy link
Contributor

@akarnokd Just to be clear, RS does not define the terms "fail-fast mode" nor "delay-error mode".
While 1.4 doesn't say "immediately", there is precedent for using eventually to describe actions which are lenient in terms of delivery. The reason for not having to do request(…) in order to get a completion signal is because TCP made that mistake—having to prod the connection to figure out if it is broken is the cause of many bugs. As for immediacy, I think it's fine to, if there's a downstream demand of X, and X…X-N items to send before propagating the onError signal. But not holding off until more demand is signalled.

Since there is no corresponding issue open against reactive-streams-jvm, I'm assuming that the above is not an issue there? If so, I'd like to understand why.

@akarnokd
Copy link
Contributor Author

akarnokd commented Apr 3, 2017

These are issues in reactive-streams-jvm as well but I discovered them only when I implemented Processors in C# to check the TCK (as stated at the very top of this issue).

Since RS is in-memory, it is possible data is buffered and is available when the final event, an onError gets posted. Implementations may chose to keep strong event ordering and thus deliver onNexts that are in the buffer before delivering the final onError. That requires downstream requests of course. Since this behavior may depend on the actual use case (sometimes its okay, sometimes the onError should cut ahead), the behavior is parametric. The problem is that the TCK always expects eager onError delivery and the same Processor implementation may pass in "fail-fast mode" and fail in "delay-error mode".

I think the best would be I post a PR to the JVM version that enables this behavior so actual Java code can be analyzed and discussed.

@akarnokd
Copy link
Contributor Author

akarnokd commented Apr 3, 2017

Posted a PR to enhance the JVM version:

reactive-streams/reactive-streams-jvm#348

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants