0.4.0 TCK includes Processor rule that was removed in #69

[danarmak]

The TCK tests this rule:

A Processor must cancel its upstream Subscription if its last downstream Subscription has been cancelled.

This rule was removed in #69 and IIUC should be removed from the TCK too.

[ktoso]

Hi, thanks for pointing this out! Seems to be a valid point, thanks!

Guys, I'm currently on holiday and traveling a lot – won't be able to follow up in detail on these until 30th, I would appreciate some fellow reactive-streams contributor helping out here... :-)

[ktoso]

btw, great to see that publishing an artifact with the TCK brings in more feedback, thanks a lot for this (and the other issue) @danarmak

[smaldini]

I missed the rule had been deleted, we use to naturally clean references too so we passed this TCK without issues. I wonder tho if the deletion is relevant as neither Subscriber or Publisher enforce really that rule. If some chaps can double check @reactive-streams/contributors if we leave the spec as it and delete the rule from TCK or if we update Publisher with that rule ?

[danarmak]

@smaldini there are good reasons for removing that rule, which were discussed in #69. There are useful behaviors for a Processor that rule would forbid.

Example: a load-balancing Processor. Worker nodes join and leave it by adding and removing subscriptions. A single publisher provides input to the processor, and each input item is distributed to exactly one subscriber. While there are no subscribers, I may want the processor to drop items, or to stop requesting them from the publisher and so pause the whole pipeline, but I don't want the whole pipeline to be canceled.

A slightly different example: a "pipe switcher". An identity Processor which serves to occasionally disconnect the bottom part of the pipeline and connect a different part in its place. Such a Processor experiences a (brief) time period when no bottom part (subscriber) is connected to it, but it shouldn't cancel its upstream, just stop requesting items.

Another example: pub-sub. If I have a hot Publisher which is expensive to set up, I would like it to keep working (discarding data) while no subscribers are connected. But every Publisher might incidentally be a Processor.

[smaldini]

I was agreeing on the optional nature of this, in fact the Publisher rule used to specify a keepAlive mechanism which we implemented therefore, similar to rabbitMQ temp queues for instance. But that rule has also been deleted (shortly after #69).

Just to be sure I am aligned on your examples: In your first one the processor would not request new items if it is cancelled or can certainly implement its own pause/drop mechanism without impacting such rule.
The second and third ones are quite similar in essence, since a cold stream could be replayed anyway (as done by RxJava and Reactor retry operations). The real bummer as I understand it is Hot Stream vs Cold Stream, and I do agree the first one is naturally a case for keepAlive true by default but that should be decided by the implementation. I liked the rule because it described this simple mechanism whilst not enforcing it, and the TCK could test both types of stream (hot vs cold) for this case.

[danarmak]

@smaldini it's not clear to me what an optional rule looks like. If you know which behavior your implementation has, then you can enable or disable the individual test. Is that the idea?

I don't remember the keepAlive rule - I stopped tracking all the changes at some point and just reread 0.4 as published.

[smaldini]

There are already a few recommendations in the specs available, cmd+f "MAY" like https://github.com/reactive-streams/reactive-streams/#1.2.

The keepAlive rule looked like this:
"When the Subscription is not cancelled, Subscription.cancel() the Publisher MUST transition to a shut-down state [see 1.17] if the given Subscription is the last downstream Subscription. Explicitly adding "keep-alive" Subscribers SHOULD prevent automatic shutdown if required."

An optional TCK is harder to enforce I agree, and there is maybe no point in ensuring through more verbose configuration options to try this extension. So in the end, removing the tck still makes sense unless we find a way to update it which as said would have little relevance. Although Doge likes free tests on his project, he can haz as many more as possible 💃

[danarmak]

What does the TCK currently do about the other MAY recommendations?

[smaldini]

For the 1.2:

 // Verifies rule: https://github.com/reactive-streams/reactive-streams#1.2
  @Required @Test
  public void spec102_maySignalLessThanRequestedAndTerminateSubscription() throws Throwable {
    final int elements = 3;
    final int requested = 10;

    activePublisherTest(elements, new PublisherTestRun<T>() {
      @Override
      public void run(Publisher<T> pub) throws Throwable {
        final ManualSubscriber<T> sub = env.newManualSubscriber(pub);
        sub.request(requested);
        sub.nextElements(elements);
        sub.expectCompletion();
      }
    });
  }

[danarmak]

I don't understand this test. If the publisher supplies fewer than elements=3 items before calling onComplete, wouldn't the test fail (with sub.nextElements(elements) timing out), despite the spec saying the publisher MAY do this?

OTOH, the Publisher you provide as part of the TCK test impl. is required to return exactly as many items as specified. In which case what is the point of this test?

[smaldini]

The publisher has 3 elements, and the test expects 3 elements received since we request 10, but it won't wait for 7 more elements to trigger completion.

[danarmak]

Sorry, I misread the test. But it's still unclear to me what the value of the test is.

To fail the test the publisher would have to provide <3 items (which is still legal). Doing anything else, like providing even >10 items, won't fail the test (unless that is implemented somewhere I didn't see). So what does this test passing prove?

[smaldini]

A failing publisher would simply blindly track the incoming requests and wait for this count to be zero (in the Subscription) before actually triggering complete signal, despite the fact the publisher only hosts 3 elements. In fact it is more a test of the subscription behavior to me in that case as I can see how it could fail to pass that rule here, but on the Publisher side you need to have a really exotic one to have a chance to miss :)

[danarmak]

OK, I understand now, thanks!

That was a bit of a digression. So since a Processor MAY unsubscribe from its publisher when its last subscriber unsubscribes (implicitly according to the current rules), what is there for a test to validate? It may unsubscribe or not as it pleases.

[smaldini]

Yeah ! no worries at all the more we talk about it the more we master it.
To the original concern, I think you are right there are little options for interpretations, @ktoso if you have any idea (so sorry to ping you!) ? We could remove the @Test annotation and leave the TCK implementor the option to override, call super and annotate with @Test in the impl ?

[viktorklang]

@smaldini Isn't it better to have the word "optional" in the name, and have it by default and then one can opt-out (otherwise it'll be less obvious what optional ones exist? and when new optionals are added.) Thoughts?

[danarmak]

@viktorklang this case isn't like an optional feature being implemented or not, where implementing is generally superior to not. This is a choice between two equally valid and useful behaviors. Indeed, some libraries will have different Processors with each of the two behaviors, to serve different needs.

If I'm right about this, then why should the TCK block one behavior by default (but always allow the other)? It seems to me to be more similar to hot vs. cold publishers, which allow many combinations of legal behavior, and the TCK doesn't enforce any one of them.

[viktorklang]

@danarmak If I understand you correctly, your point is that the TCK needn't verify optional behavior?

[danarmak]

@viktorklang If there are several alternative, equally valid behaviors, and there isn't a strong reason to think one is better than the others, or is the "default", then the TCK shouldn't enforce any of the behaviors. I think this description corresponds to the use of MAY in the spec, as opposed to SHOULD.

On the other hand, some optional behaviors are optional features and if you can test for their presence as part of the test then you can and should test that they work correctly if they are present. But this isn't such a case.

[smaldini]

@viktorklang on continuous integration we just have to make sure libraries that don't implement such optional behavior don't fail. I thought @Required purpose was specifically addressing this. Also while you're around, it seems that we also have to decide to re-add the publisher rule or not (as a MAY not a MUST).

[viktorklang]

@smaldini Do we need to re-add it? The behavior is admissible given the current spec right? Or you mean more from a guidance-perspective for the implementor? (could be a footnote?)

[smaldini]

@viktorklang yes, the behavior is admissible, a footnote maybe, but I find it important enough to highlight it somehow. I mean we obviously missed it and forgot it in TCK assuming both Akka Streams and Reactor passed it, but now the question arises. The reason also to find it important to be candidate for a MAY rule is this is in line with our general purpose to be lightweight and efficient, cleaning resources wherever possible seems to fulfill this goal, but there are still cases for not enforcing it obviously in hot streams.

[viktorklang]

@smaldini I agree with your sentiment that it is a common and useful pattern. I could be convinced to add it as a MAY clause, but it would be nice to keep the spec lean. Perhaps this is an opportunity to have an Appendix/Implementors guide to list useful behaviors like this?

[danarmak]

@viktorklang if you make such an appendix, it would be useful to include strict definitions of (maybe multiple kinds of) "hot" and "cold" publishers. These terms are often mentioned and while the general intent is clear some details are not always obvious.

[viktorklang]

@danarmak You mean if -we- make such an Appendix? :-) I agree that definitions of hot and cold Publishers is extremely important to have for the reactive streams readme.