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

fix(verifier): acquire HTTP port atomically #344

Merged

Conversation

alexeits
Copy link

@alexeits alexeits commented Sep 25, 2023

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

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

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
Copy link
Author

alexeits commented Sep 25, 2023

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
Copy link

Coverage Status

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

@YOU54F
Copy link
Member

YOU54F commented Sep 29, 2023

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 mefellows merged commit 92ac878 into pact-foundation:v1.x.x Oct 2, 2023
13 checks passed
@mefellows
Copy link
Member

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

@mefellows
Copy link
Member

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 alexeits deleted the v1-acquire-ports-atomically branch October 2, 2023 14:31
@alexeits
Copy link
Author

alexeits commented Oct 2, 2023

@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!

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

Successfully merging this pull request may close these issues.

None yet

4 participants