Replace SubstreamCancelStrategy with SupervisionDecider for Split

[mdedetrich]

This PR is the equivalent of akka/akka#31415 but for Pekko, I would recommend reading the conversation thread from that PR to get the context/justification for the feature.

The very brief summary is that the SubstreamCancelStrategy abstraction that is only used when creating a SubFlow (via usage of splitWhen and other associated functions) is not needed since Pekko already provides a generic streams abstraction for this called a SupervisionDecider. It can even be said that the SubstreamCancelStrategy is worse than pointless duplication because unlike SupervisionDecider you can't do things like log the thrown exception in the SubFlow (this is actually the main impetus for the change, because as user I had the situation where SubFlow was throwing exceptions and I had no idea what was going on because it wasn't logging them via a custom SupervisionDecider which I defined, even worse because its not possible to change the already defined SubstreamCancelStrategy you have to do annoying workarounds to log any potentially thrown exceptions in a SubFlow).

Note that this change although not source/binary breaking it does change behaviour because the default SupervisionDecider is to stop in case of an exception where as the default SubstreamCancelStrategy default is to ignore and keep on going (this is also the reason behind the test changes in the PR). As stated in the original PR, even though it is a breaking behavioural change, since it fails more explicitly its much easier for a user to notice (see akka/akka#31415 (review)).

Due to this behaviour change the PR was intended to be looked at by @raboof and @jrudolph to get buy in/opinion whether this change is acceptable (see akka/akka#31415 (comment)), would be great to get feedback on this PR's premise (because otherwise we have to create an entirely new set of functions which overall I think is worse proposition).

PR is currently draft because I didn't update the docs for the behavioural change. If @jrudolph / @raboof are happy with this change (along with anyone else that has strong experience with akka/pekko-streams, i.e. @He-Pin ) then I will update the PR with proper documentation explaining all the changes. It will also target Pekko 1.1.x.

[He-Pin]

I will read this PR in more detail, and add feedback later.

[He-Pin]

The original issue is : akka/akka#31394

[He-Pin]

Pekko-HTTP is using the splitAfter and splitWhen IIRC, how about running a snapshot against it too?

[mdedetrich]

Pekko-HTTP is using the splitAfter and splitWhen IIRC, how about running a snapshot against it too?

Sure I can do this, I would expect that pekko-http 1.1.x would depend on pekko 1.1.x so if there are any breaking changes in pekko-http it would just be for 1.1.x.

I can check locally what impact this change would have against pekko-http

[mdedetrich]

Pekko-HTTP is using the splitAfter and splitWhen IIRC, how about running a snapshot against it too?

See apache/pekko-http#371 which is related

[mdedetrich]

So I have decided to finish off this PR (which just involved updating the documentation) since it makes sense to release this as part of Pekko 1.1.0. Of note is that I had to add a new migration-guide-1.0.x-1.1.x.md which follows the same style Akka used (I copied it from https://github.com/mdedetrich/akka-apache/blob/main/akka-docs/src/main/paradox/project/migration-guides.md and https://github.com/mdedetrich/akka-apache/blob/main/akka-docs/src/main/paradox/project/migration-guide-2.5.x-2.6.x.md which is the latest Apache 2.0 release of Akka).

Regarding the behavioural changes, as you can see from https://github.com/mdedetrich/akka-apache/blob/main/akka-docs/src/main/paradox/project/migration-guide-2.5.x-2.6.x.md?plain=1#L17-L18, doing behavioural changes across minor updates do happen as long as there is a justification for it and I believe this PR does hit that threshold. These changes are clearly documented in migration-guide-1.0.x-1.1.x.md.

There is one other final note, the current implementation of this PR alias's the behaviour of Supervision.restart to Supervision.stop in the context of SubFlow because at least on the surface to me, Supervision.restart doesn't make any real sense for errors thrown in the resulting SubFlow's. If I am wrong here (either in that it should instead be aliased to Supervision.resume or that retrying in the context of SubFlow does make sense in which case I should implement it) then let me know.

[mdedetrich]

Pinging @He-Pin , @pjfanning , @jrudolph and @raboof since I forgot to do so earlier.

[mdedetrich]

@pjfanning All apostrophe's have been removed

[pjfanning]

lgtm - seems sensible to not need the SubstreamCancelStrategy. I would like to see some other approvals though.

[nvollmar]

lgtm

[He-Pin]

@mdedetrich I have left some comments

[mdedetrich]

@mdedetrich I have left some comments

I can't see any, I think you forgot to submit?

[He-Pin]

I updated my comments

[mdedetrich]

@He-Pin Responded to comments. I think that when you looked at the PR you didn't expand out the diff's and missed the fact that a lot of the code you commented on was within @InternalApi making the comments irrelevant.

[He-Pin]

I think this one is ok, I missed it's was internalApi.

[mdedetrich]

I think this one is ok, I missed it's was internalApi.

Thanks

I will leave the PR open till the end of the month (given that its christmas) to see if anyone else wants to comment on it, at that point I will merge it.

[mdedetrich]

The commit I just pushed was adding @deprecation in some places that I missed beforehand, otherwise PR is the same.

[mdedetrich]

Im going to go ahead and merge this, largely so that migration-guide-1.0.x-1.1.x.md file which this PR introduces can be used for any future possible behaviourally incompatible changes but also so I can test the changes sooner rather than later to iron out any potential problems (of particular note is pekko-http which actually uses sub flow).

[mdedetrich]

Just wanted to mention that since the changes in this PR were published as a snapshot, pekko-http tests ran against these changes and passed without any problems (see https://github.com/apache/incubator-pekko-http/actions/runs/7353051075/job/20018534864)