Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

http client fails with ring/ring-jetty-adapter 1.11.0 #250

Closed
dharrigan opened this issue Jan 8, 2024 · 10 comments
Closed

http client fails with ring/ring-jetty-adapter 1.11.0 #250

dharrigan opened this issue Jan 8, 2024 · 10 comments

Comments

@dharrigan
Copy link

dharrigan commented Jan 8, 2024

Thank you for your interest in helping to improve Cognitect's aws-api!

Dependencies

Clojure CLI version 1.11.1.1435

openjdk 21.0.1 2023-10-17 LTS
OpenJDK Runtime Environment Temurin-21.0.1+12 (build 21.0.1+12-LTS)
OpenJDK 64-Bit Server VM Temurin-21.0.1+12 (build 21.0.1+12-LTS, mixed mode, sharing)
ring/ring-jetty-adapter {:mvn/version "1.11.0"}
com.cognitect.aws/api {:mvn/version "0.8.686"}
com.cognitect.aws/cloudfront {:mvn/version "847.2.1365.0"}
com.cognitect.aws/endpoints {:mvn/version "1.1.12.626"}
com.cognitect.aws/s3 {:mvn/version "848.2.1413.0"}

Description with failing test case

As ring is now available as a 1.11.0 release (along with the dependency ring/ring-jetty-apapter 1.11.0), when upgrading deps and then starting the http-client (as a dependency injected resource), the following occurs:

user=> (dev/go)
Syntax error (NoSuchFieldException) compiling . at (cognitect/http_client.clj:41:9).
getProperties
user=> 

Reverting back to ring/ring-jetty-adapter {:mvn/version "1.10.0"} fixes the problem.

Thank you.

-=david=-

@CambodianCoder
Copy link

Encountering this issue, too. Upgrading to ring-jetty-adapter 1.11.0 causes the exception

Syntax error (NoSuchFieldException) compiling . at (cognitect/http_client.clj:41:9) getProperties

A revert back to ring-jetty-adapter 1.10.0 resolves it.
The motivation for the ring-jetty-upgrade was a detection from one of our security scanners for a CVE (which doesn't impact us; we're just aiming to resolve the alert).

@scottbale
Copy link
Collaborator

Thanks, and sorry about this frustration. I'm probably stating what you already realize, but

  • ring-jetty-adapter 1.11.0 depends upon org.eclipse.jetty/jetty-XYZ 11.0.18 artifacts
  • latest aws-api transitively depends upon org.eclipse.jetty/jetty-XYZ 9.4.51 artifacts
  • this mismatch causes problems at runtime such as what you're seeing.
  • ring-jetty-adapter 1.10.0 depends upon org.eclipse.jetty/jetty-XYZ 9.4.51 artifacts, so rolling back to it solves the runtime problems, but of course jetty 9.4.51 has that CVE.
  • clojure -X:deps tree can be used to visualize the dependencies.

Solving the problem of aws-api's transitive dependency on jetty 9.4.x is our top priority for our next release.

@ovistoica
Copy link

ovistoica commented Jan 26, 2024

Any idea when the transitive dependency will be solved? Currently we cannot use aws-api as we transitioned to https://github.com/sunng87/ring-jetty9-adapter which uses Jetty 12.0.x and we cannot downgrade or switch back to ring adapter as we rely on functionality from Jetty 12.0

I saw there was an effort to move the HTTP client to the JDK version

As an aside, Is there any example code of how to use the :http-client prop so we don't rely on the default-http-client?. I see it needs to implement the HttpClient protocol

(defprotocol HttpClient
  (-submit [_ request channel]
    "Submit an http request, channel will be filled with response. Returns ch.

     Request map:

     :scheme                 :http or :https
     :server-name            string
     :server-port            integer
     :uri                    string
     :query-string           string, optional
     :request-method         :get/:post/:put/:head/:delete
     :headers                map from downcased string to string
     :body                   ByteBuffer, optional
     :timeout-msec           opt, total request send/receive timeout
     :meta                   opt, data to be added to the response map

     content-type must be specified in the headers map
     content-length is derived from the ByteBuffer passed to body

     Response map:

     :status            integer HTTP status code
     :body              ByteBuffer, optional
     :headers           map from downcased string to string
     :meta              opt, data from the request

     On error, response map is per cognitect.anomalies.

     Alpha. This will absolutely change.")
  (-stop [_] "Stops the client, releasing resources"))

@tobias
Copy link

tobias commented May 6, 2024

I have a fork of com.cognitect/http-client that uses Jetty 11: https://github.com/tobias/cognitect-http-client. I'm currently using it in production for clojars.org without issue. Folks are free to use that, and @scottbale you are welcome to use the changes there if you do decide to upgrade the official client to Jetty 11.

@bhurlow
Copy link

bhurlow commented Aug 7, 2024

+1 on this, we can't use aws-api with https://github.com/hyperfiddle/electric since it requires Jetty 11 for websocket support

@tobias
Copy link

tobias commented Aug 7, 2024

@bhurlow Does my fork work for you?

@reify-kurt-wheeler
Copy link

Solving the problem of aws-api's transitive dependency on jetty 9.4.x is our top priority for our next release.

Hello, it's been about 7 months since this comment. Is there any solution for this issue yet?

@terjesb
Copy link

terjesb commented Aug 28, 2024

Hello, it's been about 7 months since this comment. Is there any solution for this issue yet?

(In the mean time, for us Tobias' fork worked fine on Jetty 11. Jetty 11 reached end of community support as of January 2024. Per #181 (comment), we forked Tobias' Jetty 11 fork and made it work with Jetty 12.)

@scottbale
Copy link
Collaborator

We have just released a beta release which introduces a Java-native http client. With this release, Jetty is no longer by default a transitive dependency.

@dharrigan if time permits, could you please verify whether this release solves your issue? Thanks.

(I will leave this issue open for the time being until we verify.)

@scottbale
Copy link
Collaborator

Fixed in version 0.8.711

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

8 participants