clj-commons / aleph

Asynchronous streaming communication for Clojure - web server, web client, and raw TCP/UDP
http://aleph.io
MIT License
2.54k stars 241 forks source link

Include Brotli and Zstd deps - 🎄 #705

Closed KingMob closed 6 months ago

KingMob commented 6 months ago

They're now included by default, and all the code to check whether they're present has been removed, and replaced with a static check that will blow up if they're somehow missing.

KingMob commented 6 months ago

Fixes #703

KingMob commented 6 months ago

@DerGuteMoritz @arnaudgeiser Any thoughts, or shall we merge it?

David-Ongaro commented 4 months ago

Why does it have to be a required dependency? Using a baseimage with the native libs included increases our image size by about 40 MB, and since we use aleph within a service-mesh we probably will never use brotli compression.

KingMob commented 3 months ago

@David-Ongaro I actually meant to make it optional to begin with. But the Clojure compiler itself had some bugs in supporting Netty's optional codec-checking code.

Here's a little context on it from Slack:

alexyakushev I understand. Unfortunately, we have limited resources, and supporting modern codecs without requiring the deps started consuming more effort than I'd like, and I'm concerned about future maintenance burden. I originally designed it all to keep the codec deps optional, but that's trickier than it looks, it turns out.

At its root, the issue is the Clojure language uses the same syntax for static member access and static function calls, and the compiler requires reflection to distinguish between the two. Twice this led to tricky ClassNotFoundExceptions compiling codec interop code (that Java itself has no problem with). I ended up writing a whole Java class just to provide a Clojure-safe facade for the problem. Despite that, the problem cropped up again in a new form. While I think I can still make the codecs optional, both choices started to seem equal overall, so I thought it worth asking the users if they had a preference.

Other consequences: for Brotli, it's effectively supported everywhere, and I have no problem making it a default, and if optional, fewer people would enable it. Zstd's future is less clear, but including the Brotli deps and not the Zstd one would just be the worst of both worlds.

Dependency conflicts are always a possibility, but that seems unlikely in this case. People with pre-existing Br/zstd deps are probably trying to modernize Aleph, if I had to guess.

David-Ongaro commented 3 months ago

@KingMob Thanks for posting this context for the benefit of those who are not members of this Slack channel. That makes the reasoning much more understandable. I couldn't imagine that it's so complicated to support an optional dependency. It seems in Netty itself even HTTP/2 support is optional, but that is pure Java...

Still, the argument of browser support makes me wonder if wider use cases are taken into consideration during Aleph development. This point is irrelevant when using Aleph within a service-mesh. I just hope this doesn't lead to a future impedance mismatch.

KingMob commented 3 months ago

I couldn't imagine that it's so complicated to support an optional dependency.

We would have had to maintain a complete Java facade for the relevant classes, ensure that the Clojure compiler never reflects on that code or we'd get an NPE, maintain a separate testing regime, etc. It's doable, but keeping it optional would have added to our burden, and Aleph is nobody's full-time job. Heh, if I compared the hours worked last year to the grant I got from Clojurists Together, I probably made below minimum wage working on Aleph.

Still, the argument of browser support makes me wonder if wider use cases are taken into consideration during Aleph development.

Well, we almost never hear from people, so I don't know how others are using Aleph. That makes it hard to account for their needs when making decisions. You said yourself, you don't follow the slack channel. And unlike a lot of project maintainers, I do try to solicit feedback. This very question was put to a vote in the #aleph slack channel.

If artifact size is that crucial to your company, I'd suggest maintaining a fork.

David-Ongaro commented 3 months ago

Heh, if I compared the hours worked last year to the grant I got from Clojurists Together, I probably made below minimum wage working on Aleph.

I understand, and I'll be the first to suggest Aleph if we get a budget available to support open source Clojure projects.

Well, we almost never hear from people

I guess that's the curse of open source, as long as it works no one says a single word...

If artifact size is that crucial to your company, I'd suggest maintaining a fork.

It's not, it's a nice to have. And we almost recouped that increase by just taking a closer look at our Dockerfile and avoid unnecessary layers... The wider concern is avoiding unnecessary dependencies and complexity. But the SAP Concur Clojure, group is not big (sadly) so we surely won't maintain our own fork, and we probably won't get resources contributing.

KingMob commented 3 months ago

@David-Ongaro Another possibility occurred to me that should be lower maintenance than a fork. What if you add a build step to strip out the architectures you don't need from the jar file?

David-Ongaro commented 3 months ago

What if you add a build step to strip out the architectures you don't need from the jar file?

I figured Leiningen actually has an option for that: :uberjar-exclusions. So I tried that out, and it indeed reduces the jar size by about 11 MB, so it reclaims about a fourth of the 40 MB (the rest being due to having to use a different base image in order to support brotli, libstdc++ etc.). But as indicated above, the image size is just a symptom, with the real concern being additional dependencies which may imply future CVEs etc.

In any case, we can live with that. Much more relevant is that Aleph is actively maintained. In that sense, we're happy with the new 0.7 releases.

DerGuteMoritz commented 3 months ago

@David-Ongaro Took antoher stab at it: https://github.com/clj-commons/aleph/pull/723 -- since the workaround is pretty simple, I think we could justify making the dependencies optional again. But let's see how the others feel about it :slightly_smiling_face:

David-Ongaro commented 3 months ago

That's looks promising! Let's hope it works out.