jbwheatley / pact4s

Apache License 2.0
29 stars 11 forks source link

[Verifications] java.net.BindException: Address already in use #674

Open gaeljw opened 1 month ago

gaeljw commented 1 month ago

Issue

In provider verifications tests, the StateManagementFunction uses a fixed port to start an HTTP server.

In shared ("non isolated") CI environments, this cause issues because two tests running at the same time are likely to conflict.

Example of error:

[info] - should verify pacts *** FAILED ***
[info]   java.net.BindException: Address already in use
[info]   at java.base/sun.nio.ch.Net.bind0(Native Method)
[info]   at java.base/sun.nio.ch.Net.bind(Net.java:555)
[info]   at java.base/sun.nio.ch.ServerSocketChannelImpl.netBind(ServerSocketChannelImpl.java:337)
[info]   at java.base/sun.nio.ch.ServerSocketChannelImpl.bind(ServerSocketChannelImpl.java:294)
[info]   at java.base/sun.nio.ch.ServerSocketAdaptor.bind(ServerSocketAdaptor.java:89)
[info]   at jdk.httpserver/sun.net.httpserver.ServerImpl.<init>(ServerImpl.java:142)
[info]   at jdk.httpserver/sun.net.httpserver.HttpServerImpl.<init>(HttpServerImpl.java:50)
[info]   at jdk.httpserver/sun.net.httpserver.DefaultHttpServerProvider.createHttpServer(DefaultHttpServerProvider.java:35)
[info]   at jdk.httpserver/com.sun.net.httpserver.HttpServer.create(HttpServer.java:150)
[info]   at pact4s.StateChanger$SimpleServer.start(StateChanger.scala:50)
[info]   ...

Relevant code in pact4s: https://github.com/jbwheatley/pact4s/blob/de31eca1990495b048cc16cdac7b32c124d0cd4f/models/src/main/scala/pact4s/provider/StateManagement.scala#L59

Example of code for the record:

import pact4s.scalatest.PactVerifier

class PactVerificationsTest extends ... with PactVerifier {

  ...

  override def provider: ProviderInfoBuilder = ProviderInfoBuilder(PactComponentName, PactBrokerSource)
    // ...
    .withStateManagementFunction {
      StateManagementFunction {
        case ProviderState("Some state", params) =>
          // ...
          ()
      }
        .withBeforeEach(() => beforeEachContract())
    }

  "My provider" should {
    "verify pacts" in {
      verifyPacts(...)
    }
  }

Proposed solution

Even though shared CI environments are not a best practice, they do exist and pact4s should probably pick a random available port.

jbwheatley commented 1 month ago

you can override the port with withStateChangeFunctionConfigOverrides to avoid clashes

gaeljw commented 3 weeks ago

@jbwheatley don't you think it should be the default behaviour to pick a random port?

gaeljw commented 3 weeks ago

I had a very quick look, I would need to dig a bit more to see if this could work..

The HTTP server is actually already able to pick a random port by itself if we pass it the port = 0: https://github.com/jbwheatley/pact4s/blob/de31eca1990495b048cc16cdac7b32c124d0cd4f/shared/src/main/scala/pact4s/StateChanger.scala#L50

But the chosen random port is not propagated and 0 is assumed to be the port in https://github.com/jbwheatley/pact4s/blob/de31eca1990495b048cc16cdac7b32c124d0cd4f/models/src/main/scala/pact4s/provider/StateManagement.scala#L51 which then leads to errors like State Change Request Failed - Connect to http://localhost:0/ [localhost/127.0.0.1, localhost/0:0:0:0:0:0:0:1] failed: Connection refused.

If we're able to propagate the random port chosen, I don't see why we wouldn't do this by default?

jbwheatley commented 3 weeks ago

I wrote it that way originally because it was just easier to set a default port that to worry about propagating it and having more random vars being set all over the place tbh. I figured it would be good for 99.9% of cases and then offer an override if people just happened to be binding the default port. Its fairly common (I think?) for services to set a default port of operations then allow an override, so I'm not convinced its worth your time to redo it when we already have a way for folks to get around conflicts 🤷

jbwheatley commented 3 weeks ago

but lets leave this issue open to see if anyone else feels differently!