search

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

byte-streams / manifold incompatibility. #569

Closed dspearson closed 3 years ago

dspearson commented 3 years ago

The latest alpha releases of aleph depend on alpha-release manifold, which have a new 4-arity manifold.executor/thread-factory. However, the documentation examples use byte-streams/to-string and byte-streams depends on a manifold version that does not have the 4-arity function available. This makes the examples fail.

When Zach was the maintainer of all three projects it was probably easier to keep an eye on these inter-dependencies, since he was the author, but now that it's under clj-commons stewardship it's more easily overlooked. I am unsure whether to bother making a pull request for byte-streams, bumping the manifold version, before a new non-alpha manifold release is cut. Seems better to keep them on stable point releases. So, I'm not sure what to suggest, but as of the latest version the docs don't reflect a current working state.

Example:

access-packages.core> (-> @(http/get "https://google.com/")
                          :body
                          bs/to-string
                          prn)
Execution error (ArityException) at aleph.netty/enumerating-thread-factory (netty.clj:797).
Wrong number of args (4) passed to: manifold.executor/thread-factory
KingMob commented 3 years ago

@dspearson Thanks for bringing this up. We inherited the alpha status of these libs from Zach, but you're right that since the maintainers are no longer the same person, the discrepancy might have gotten overlooked when we got around to resolving it all.

I know that, for Manifold at least, a few people are using the alpha versions for badly-needed bugfixes. I'd planned to cut a 0.1.9 point release in the past month, but life intervened. I'll be sure to bump up byte-streams's version when I do.

KingMob commented 3 years ago

@dspearson Manifold 0.1.9 has just been released. If you want to submit a PR for byte-streams to fix this, that'd be appreciated. Otherwise, I'll try to get to it when I get a moment.

KingMob commented 3 years ago

@dspearson I'm working with Erik to cut a new stable release for byte-streams. It should be out soon.

I went to revisit this, the original issue, but I tested it in the REPL without updating the byte-streams dependency, and at least on the latest master, I have no problems executing the README examples. It's possible something weird has happened, but can you double-check on your end?

dspearson commented 3 years ago

The issue I saw was with:

  :dependencies [[org.clojure/clojure "1.10.3"]
                 [byte-streams "0.2.5-alpha2"]
                 [aleph "0.4.7-alpha7"]]

It's likely that order of dependencies matter.

dspearson commented 3 years ago

Actually,I can reproduce it with the following:

  :dependencies [[org.clojure/clojure "1.10.3"]
                 [org.clj-commons/byte-streams "0.2.8"]
                 [aleph "0.4.7-alpha7"]]
> @(http/get "https://google.com/")
Execution error (ArityException) at aleph.netty/enumerating-thread-factory (netty.clj:797).
Wrong number of args (4) passed to: manifold.executor/thread-factory

Just having that order of dependencies results in failure. Switching aleph and byte-streams around results in success.

Having the manifold versions match in both libraries is likely a successful mitigation. I'll make a pull request for aleph.

mach-kernel commented 1 year ago

Same issue with these versions:

  :dependencies [[org.clojure/clojure "1.11.1"]
                 [org.clj-commons/byte-streams "0.3.1"]
                 [aleph "0.6.0"]]
Execution error (ArityException) at aleph.netty/enumerating-thread-factory (netty.clj:1027).
Wrong number of args (5) passed to: manifold.executor/thread-factory
KingMob commented 1 year ago

@mach-kernel Looks like the manifold versions are out of sync between byte-streams and Aleph again.

For now, if you're trying to use the latest byte-streams, ysk Aleph 0.6.0 also uses it, so you can remove byte-streams from your explicit deps, and it should all work.