do[On|Before|After] to use Supplier<..>

[Scottmitch]

We have operators that allow for invocation of Consumers and Runnables at different points in the life cycle of a Subscription/Subscriber. Most of these methods do not take a Supplier and that means if the corresponding Consumer or Runnable intended to have state per-subscribe they will not be able to easily do so correctly. We should consider if updating these methods to require a Subscriber makes sense.

DoBeforeFinallyOnHttpResponseOperator is another operator that shares the same issue.

https://github.com/servicetalk/servicetalk/pull/319#discussion_r

[NiteshKant]

do* operators are typically side-effect operators, i.e. they do not carry state per subscribe but create a side-effect, eg: logging, incrementing counters, etc. Having these operators use a supplier make them more complex for typical usage:

.doFinally(() -> responseProcessedLatch::countDown)

vs

.doFinally(responseProcessedLatch::countDown)

Perhaps DoBeforeFinallyOnHttpResponseOperator is badly named since it's typical usecase is to capture state per subscribe, eg: request state, tracing, etc.

[Scottmitch]

@jayv ^

[jayv]

I'm all for finding a better name for DoBeforeFinallyOnHttpResponseOperator, I still think it makes sense to have both options, because in cases where there is state in the callback you don't need to wrap the operator in an extra state capturing lambda [*] you pass to Single.defer() if all you need is a simple fresh copy of your context object per subscription.

[*] assuming the context doesn't need to be passed into multiple operators on the same source, then you need to create it outside the scope of the operator.

[NiteshKant]

An operator to look at is using to introduce state per subscribe that is disposable. This can generally be applied to any execution chain and is more useful than having overloads per operator to manage state per subscribe.

[jayv]

@NiteshKant Conceptually I think I get what it wants to do, but not sure how that would work in practice. Can you rewrite this example using a made up using() operator? I'm not sure I see how it translates to the below use-case, in particular how do I interact with the resource that's created behind the scenes from the factory in my operator downstream.

return Single.deferShareContext(() -> {
            final ScopeTracker scopeTracker = newTracker(request, singleSupplier);
            final Single<StreamingHttpResponse> responseSingle;
            try {
                responseSingle = scopeTracker.prepareScopeAndRequest();
            } catch (Throwable throwable) {
                return Single.error(throwable);
            }
            return responseSingle
                    .liftSynchronous(new DoBeforeOnFinallyOnHttpResponseOperator(scopeTracker))
                    .doBeforeSuccess(scopeTracker::onResponseMeta);
        });

[NiteshKant]

(To be sure we are talking the same thing, this issue is to introduce overloads for do* on our sources and not for DoBeforeOnFinallyOnHttpResponseOperator specifically, although the concepts may apply)

The signature of using will roughly be:

public static <T, R> Single<T> using(Supplier<R> resourceFactory,
          Function<R, Single<T>> singleSupplier, 
          Consumer<R> resourceDestructor)

The code you shared is a bit more complicated because of DoBeforeOnFinallyOnHttpResponseOperator which spans across two resources. If we add using DoBeforeOnFinallyOnHttpResponseOperator will transform to using() signature FWICT.

Assuming a simple Single, your sample will be converted to:

return Single.using(() -> newTracker(request), 
       scopeTracker -> {
               final Single<StreamingHttpResponse> responseSingle;
               try {
                   responseSingle = scopeTracker.prepareScopeAndRequest();
               } catch (Throwable throwable) {
                   return Single.error(throwable);
               }
               return responseSingle
                    .liftSynchronous(new DoBeforeOnFinallyOnHttpResponseOperator(scopeTracker))
                    .doBeforeSuccess(scopeTracker::onResponseMeta);
               }, scopeTracer::destroy);

[jayv]

Yeah feels like a toss up with the existing code, maybe slightly less appealing, seems like we can't get around a capturing lambda either way, so Single.defer(..) seems more flexible than restricting us to what using(..) provides.

I think I'm convinced that Supplier alone may not be sufficient for the general do*-opers and it also doesn't help with the current DoBeforeOnFinallyOnHttpResponseOperator use-cases.