WebSockets Next: get rid of UniHelper#toUni() #41467
Conversation
mkouba
commented
Jun 26, 2024
mkouba
commented
- and replace it with Uni.createFrom().completionStage(Supplier)
- we need lazy subscription of the produced Uni
- and replace it with Uni.createFrom().completionStage(Supplier) - we need lazy subscription of the produced Uni
| @@ -144,7 +143,7 @@ public Uni<WebSocketClientConnection> connect() { | |||
| throw new WebSocketClientException(e); | |||
| } | |||
|
|
|||
| return UniHelper.toUni(client.connect(connectOptions)) | |||
| return Uni.createFrom().completionStage(() -> client.connect(connectOptions).toCompletionStage()) | |||
There was a problem hiding this comment.
Should we replace the lambda with an anonymous class?
There was a problem hiding this comment.
I'm sorry but no. The reactive code is difficult to read even with lambdas. Also we use lambdas everywhere in this extension. How do you determine where to use lambda and where an anonymous class? I know that we used to avoid lambdas where possible due to some memory overhead but (1) I'd like to know if it's still the case for new JDKs and (2) I don't want to spend too much time on performance until we have some reasonable benchmarks.
There was a problem hiding this comment.
-1 for everyrhing mentioned above
There was a problem hiding this comment.
-1 for everyrhing mentioned above
Any arguments?
There was a problem hiding this comment.
The argument is simple: It's been a (loose) policy all along, so if we want to change it, we should do the bare minimum of trying to figure out whether it makes sense to or not, not just put forth generic arguments that could be applied to almost any situation.
There was a problem hiding this comment.
The argument is simple: It's been a (loose) policy all along...
Yes, a very "loose" policy and I personally viloated this policy many times when dealing with async/reactive code ;-)
we should do the bare minimum of trying to figure out whether it makes sense to or not, not just put forth generic arguments that could be applied to almost any situation.
Readability/maintenance cost is IMO not a generic argument but I agree that we should verify whether the assumptions for this policy still apply.
@franz1981 do you happen to know if the assumption that lambdas bring significant performance overhead (memory, cpu, allocation, etc.) compared to annonymous classes still holds true?
I can try to prepare a simple (and naive) benchmark to verify this assumption...
@geoand I will merge this PR because we already have a bunch of lambdas in this extension but I've added a note to the #39148. If we decide to get rid of the lambdas (based on our findings) then it should be done globally (in the scope of the extension).
Status for workflow
|
