Make it clearer that HttpResponse construction should generally not block

[anuraaga]

This is just food for thought, can close if we're happy already.

I find it's too easy for users to shoot themselves in the foot since the difference in "semantic meaning" of an HttpResponse (something that streams) and AggregatedHttpResposne (something that's collected) is very subtle.

openzipkin-contrib/zipkin-storage-kafka@c5d6976

Just changing all references of HttpResponse to AggregatedHttpResponse compiles, but changes the behavior completely from not-work to work. This is very subtle.

Should we reconsider providing the helpers that return a HttpResponse based on collected payloads? Remove from(CompletableStage<HttpResponse>) in favor of from(CompletableStage<AggregatedHttpResponse>? Change the naming? Or maybe just do nothing :)

[trustin]

I'm looking into openzipkin-contrib/zipkin-storage-kafka@c5d6976 but I don't see why the previous version does not work. Could you elaborate? Internally, an AggregatedHttpResponse will be translated into an HttpReaponse anyway.

[anuraaga]

For an annotated service when returning an HttpResponse the logic is run on the event loop but when returning an aggregated one it's run on the blocking executor. This seems to make sense to me since one is streamable, but easy to accidentally use HttpResponse, make blocking calls, and deadlock.

[trustin]

when returning an HttpResponse the logic is run on the event loop but when returning an aggregated one it's run on the blocking executor.

Oh, I never knew returning an AggregatedHttpResponse makes the service method run on the blocking executor. I think it's a bug and they should both run in an event loop thread. Such behavioral difference is so subtle that I would like to avoid.

[anuraaga]

I guess we need an annotation then to control the executor :)

[trustin]

Yeah. Something like @Blocking("taskType" or BlockingTaskType.XXX) ?

[anuraaga]

Yeah seems good. But I think most code right now returns json and such, which also run on blocking (HttpResponse and CompletionStage run on event loop). Is it too huge of a behavior change?

[trustin]

I guess it's fine as long as we make it clear in our release note? 😅

[anuraaga]

I guess so :) But we might consider adding a version that logs warnings that users need to add the annotation before changing - if it were just a class load exception or something it's still easy to debug if you missed the release notes, but if it's deadlocks people could get very confused.

[trustin]

Created: #2078

[anuraaga]

Thanks! I'll close this one then.