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

Java API for bindAndHandle does not allow port 0 #660

Closed
akara opened this issue Dec 15, 2016 · 3 comments · Fixed by #803
Closed

Java API for bindAndHandle does not allow port 0 #660

akara opened this issue Dec 15, 2016 · 3 comments · Fixed by #803
Labels
3 - in progress Someone is working on this ticket t:java Issues related to the Java DSL t:server Issues related to the HTTP server
Milestone

Comments

@akara
Copy link

akara commented Dec 15, 2016

We use port 0 during tests extremely frequently. As we do parallel tests, this allows Akka-HTTP to find me an open ephemeral port and bind there. We then read the actual port back from the resulting ServerBinding.

When writing Java API tests, we just found out it does not accept 0 for bindAndHandle. To be more specific, The ConnectHttp.toHost("localhost", 0) throws an exception - not the bindAndHandle itself. The culprit is in this code in ConnectHttp:

  def toHost(host: String, port: Int): ConnectHttp = {
    require(port > 0, "port must be > 0")
    val start = if (isHttpOrHttps(host)) host else s"http://$host"
    toHost(Uri.create(start).port(port))
  }

I think the second line should rather spell:

    require(port >= 0, "port must be 0 or higher")
@akara
Copy link
Author

akara commented Dec 15, 2016

Apparently there is another place that wants port > 0 in ConnectHttp:

  private def effectivePort(scheme: String, port: Int): Int = {
    val s = scheme.toLowerCase(Locale.ROOT)
    if (port > 0) port
    else if (s == "https" || s == "wss") 443
    else if (s == "http" || s == "ws") 80
    else throw new IllegalArgumentException("Scheme is not http/https/ws/wss and no port given!")
  }

jonas added a commit to jonas/akka-http that referenced this issue Jan 22, 2017
Changes ConnectHttp to support port zero indicating that a random port
should be used, which is the behavior supported by the Scala bind API.

Tested in the Scala REPL using:

    import com.typesafe.config.ConfigFactory
    import akka.http.javadsl.server.HttpApp
    import akka.http.javadsl.settings.ServerSettings

    object Server extends HttpApp {
      protected override def route = complete("ok")
      def run() = startServer("localhost", 0, ServerSettings.create(ConfigFactory.load()))
    }

    Server.run
@jonas jonas added 3 - in progress Someone is working on this ticket t:java Issues related to the Java DSL t:server Issues related to the HTTP server labels Jan 22, 2017
@jonas
Copy link
Member

jonas commented Jan 22, 2017

Thanks for reporting this. I've proposed a PR (#803) to remove this inconsistency.

@akara
Copy link
Author

akara commented Jan 24, 2017

Thanks for accepting! Looked at #803. Looks good!

jrudolph pushed a commit that referenced this issue Jan 26, 2017
* Fix ConnectHttp.toHostHttps when no scheme is given

Also documents the toHost API and unifies parsing and construction of
URIs from the host parameter.

* Allow Java bind API to specify port zero (#660)

Changes ConnectHttp to support port zero indicating that a random port
should be used, which is the behavior supported by the Scala bind API.

Tested in the Scala REPL using:

    import com.typesafe.config.ConfigFactory
    import akka.http.javadsl.server.HttpApp
    import akka.http.javadsl.settings.ServerSettings

    object Server extends HttpApp {
      protected override def route = complete("ok")
      def run() = startServer("localhost", 0, ServerSettings.create(ConfigFactory.load()))
    }

    Server.run
@jrudolph jrudolph added this to the 10.0.3 milestone Jan 26, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3 - in progress Someone is working on this ticket t:java Issues related to the Java DSL t:server Issues related to the HTTP server
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants