SyliusResourceBundle icon indicating copy to clipboard operation
SyliusResourceBundle copied to clipboard

Review Netty 5 buffer API

Open danielcompton opened this issue 3 years ago • 2 comments

There's a proposed new buffer API for Netty 5: https://github.com/netty/netty-incubator-buffer-api/blob/070ffa26885ee078f364ad0334c9b177442f370b/RATIONALE.adoc. We should take a look at it and see if it will affect Aleph/Manifold and provide comment if so.

danielcompton avatar Feb 15 '21 18:02 danielcompton

I've been thinking for a while about what the transition to Netty 5 [1][2] might look like in Aleph. I didn't want to write anything before most the major issues have been closed.

I will deliberately provoke a bit of a debate here with my current take:

Long story short We should not even try.

A little bit longer Netty 5 ByteBuffer API changes will be impactful as we'll have to take care of the raw-stream? mode we expose to not break Aleph users. There are also the ChannelHandler changes that will need to be considered. While doable, I'm wondering if it worth the effort considering those changes will most likely impact a large part of the code base.

Netty 5 is actually a good opportunity to revisit the choices that have been made we regret today:

  • Strongly encapsulate the internals (we are always reluctant to touch any public functions that should not be used outside of Aleph. Today, those are marked as no-doc but still cannot be touched)
  • Remove some parts of the API that should not be used anymore
  • Revisit some of the defaults (multipart comes naturally to mind, but it might be also the case for the transport, netty parameters and so on...)
  • Do not use Dirigiste Executors by default and definitely move away from Dirigiste Pools.
  • Do not rely on manifold Deferred classes but on CompletableFuture instead
  • Do not rely on manifold to create the threads. The magical behavior is only understood by a few of us and users are surprised about those subtleties (tied to the previous point actually)
  • Still consider manifold Stream as a default implementation but make them returning CompletableFuture instead. I still think a push-based streams approach is the most appropriate and I don't have any alternative in mind. Maybe we should expose a protocol that "implementers" can rely on.
  • Still keep an asynchronous approach by default but consider virtual threads (aka Loom) as a first class citizen. Or consider the later as default as I expect some of you would prefer.
  • Make sure that client is fully clj-http compliant without any difference
  • Provide solutions for the issues that are currently open where it would be easy to provide a solution if backwards-compatibility was not required
  • (Fix everything that will be discovered when implementing HTTP/2 support and UNIX domain socket)

To summarize : It's a good opportunity to consider beth at that point but I would wait for HTTP/2 and the UNIX domain sockets to land :)

Thoughts?


EDIT

Yes, somehow it also implies considering multifold (aka manifold v2)

[1] : https://github.com/netty/netty/pull/11347 [2] : https://netty.io/wiki/new-and-noteworthy-in-5.0.html

arnaudgeiser avatar Apr 03 '23 13:04 arnaudgeiser

I agree with almost all of this. Netty 5 introduces quite a bunch of changes, and I agree that new names would be warranted. Also, Loom virtual threads kind of throw a wrench in the entire event-driven paradigm. The value of libs like Manifold, and the event-driven parts of Netty, are in question.

Do not rely on manifold Deferred classes but on CompletableFuture instead

Agreed, although as of today, Deferreds now implement CompletionStage, courtesy of @figurantpp. 😄 But yes, a replacement Manifold should probably extend CompletableFuture to protocols, rather than roll its own.

I still think a push-based streams approach is the most appropriate and I don't have any alternative in mind

I think we should also consider reactive-streams' pull-based streams. On the upside, they neatly solve the backpressure problem automatically, as well as upstream cancellation, which we've never offered. On the downside, they complicate fan-out, and we'd be able to reuse very little existing code.

Ditto for vthreads.

Make sure that client is fully clj-http compliant without any difference

I need to finish that PR...

It's a good opportunity to consider beth

Beth/Bet/Vet would be the next letter in the Hebrew alphabet, but I think those names will be confusing, since they're also English words. We could keep the same letter, but switch languages, so it would be alif/alpha/olaf. And of course, omega and its variants is a possibility. For manifold's sequel, I thought "myriad" would be a good name...

KingMob avatar Apr 05 '23 06:04 KingMob