Resolves: #29467
Comment From: pivotal-cla
@wplong11 Please sign the Contributor License Agreement!
Click here to manually synchronize the status of this Pull Request.
See the FAQ for frequently asked questions.
Comment From: pivotal-cla
@wplong11 Thank you for signing the Contributor License Agreement!
Comment From: sdeleuze
@rstoyanchev This PR looks like to workaround my concern at least partially, but it would have been cleaner to be able to reuse CoroutinesUtils#invokeSuspendingFunction(java.lang.reflect.Method, java.lang.Object, java.lang.Object...) but for HttpServiceProxyFactory use case it looks like we don't have any target object available in the MethodInvocation. Is it by design because no instance is provided by the end user or could we find a way to provide a target object to this method via some refinements?
That said, since here for typical use cases no implementation is provided by the end user, I think it is fine to not support suspend fun foo(): Flow<String> since both suspend fun foo(): String and fun foo(): Flow<String> are supported, since the need for the former is typically required for end user implementation calling suspending functions before creating the Flow instance.
So my ask is primarily to have cleaner code and reuse existing logic if possible. Otherwise, we should be fine with a refined version of this PR.
Comment From: sdeleuze
After discussing with @rstoyanchev, 6a6a35a0b95ac946c66c102ff0a43f0a638b5421 could potentially be a source of inspiration for refining this issue.
Comment From: sdeleuze
For that use case, I think the original approach was the right one since we are intercepting an invocation within a suspending function, and given the fact there is no user function implementation involved, custom handling of suspending function returning flow makes little sense here.
I moved the implementation to a KotlinDelegate inner class to avoid classpath issues and to avoid exposing a new public API in CoroutinesUtils since this is currently the only use case we have.