search

pact-foundation / pact-go

Golang version of Pact. Pact is a contract testing framework for HTTP APIs and non-HTTP asynchronous messaging systems.
http://pact.io
MIT License
831 stars 104 forks source link

fix(verifier): acquire HTTP port atomically #344

Closed alexeits closed 8 months ago

alexeits commented 8 months ago

Acquire the port in VerifyMessageProvider and VerifyProvider atomically by using the same listener for starting the HTTP server that was used for a free port acquisition.

This prevents a race condition when multiple pact verifiers run in parallel and compete for free ports, which could result in errors like

Expected server to start < 10s. Timed out waiting for http verification proxy on port 34425 - check for errors
alexeits commented 8 months ago

Hi @mefellows, long time no chatting!

I wonder if you would entertain this change for v1.x.x? We aren't on 2.x.x yet for a variety of reasons.

We bumped into this issue when migrating our CI to a different provider. Prior to the move we experienced the error due to a port acquisition failure in pact provider verification only occasionally and rebuilding usually solved the problem. With the new builders we can't pass any builds anymore. I hope the fix makes sense and I appreciate you considering it.

Thanks, Alex

coveralls commented 8 months ago

Coverage Status

coverage: 77.392% (-0.1%) from 77.52% when pulling f7c3d13f4cdb649029efb4ce6594d82ca8658f71 on sagansystems:v1-acquire-ports-atomically into c4aa28ecca8f4ff1cd9a92456a2daa14326e1d4e on pact-foundation:v1.x.x.

YOU54F commented 8 months ago

Hey I don't any reason why we can't support this on the v1.x.x branch

Have you been able to test the changes in your own project (against your fork) to ensure its performing as you expect?

mefellows commented 8 months ago

Thanks Alex, we can bring this in and release as needed - also, nice to hear from you again :)

mefellows commented 8 months ago

The release seems to have worked, but perhaps we have a flakey build on the 1.x.x branch. I tested locally (MacOSX) and it works, so I think the change is sound.

alexeits commented 8 months ago

@YOU54F, @mefellows: yes, we are successfully running off our fork. Now, since it's merged into the upstream we'll point to the HEAD of the upstream v1.x.x instead. Thank you so much for the review!