LukeMathWalker / wiremock-rs

HTTP mocking to test Rust applications.
Apache License 2.0
607 stars 69 forks source link

Unexpected errors when more than 1 MockServer is started per test #120

Closed TooManyBees closed 7 months ago

TooManyBees commented 1 year ago

I have a testing scenario where I need to mock two external services, so in my project's test suite I started two MockServers per test. This worked fine until my test suite reached a certain level of concurrency (~20 tests on 4 threads) at which point each request which was handled by a mock server had a chance of not being recognized and returning a 404 response (according to tracing output, the error was "Got unexpected request"). I didn't see any mention in documentation of limiting the number of mock servers per test, so I assume this is a bug.

One workaround for this was to run the tests on a single thread with cargo test -- --test-threads 1. The other workaround was to use a single MockServer for both of my external services. This is the workaround that I chose to solve the issue because for my use case, one external service exclusively accepts GET requests, and the other exclusively accepts POSTs.

What I imagine happend was that a single mock server address was given to more than one test at the same time, so the second test made a request to a "dangling" TCP server whose mocks had been reset inbetween tests, but I wasn't able to prove this. When I tried to make a minimal test case to reproduce the issue, my tests instead failed with a different error ("hyper::server::tcp: accept error: Too many open files (os error 24)", on MacOS). The same workarounds made that error go away, though.

LukeMathWalker commented 1 year ago

Can you provide a reproducible example for me to troubleshoot?

TooManyBees commented 1 year ago

Well, here is the reproduction code that I used, bearing in mind that its tests failed with a different error (too many open files) than the one I'm reporting https://github.com/TooManyBees/wiremock-concurrency. I'm not sure how helpful it will be, but as I mentioned the same workaround fixed tests in both the minimal and real codebases.

I am presuming that if my minimal repro example contained enough unrelated business logic, it would slow down the frequency of network requests enough to not trigger that networking error, but still produce the error that I'm reporting. (In my real app, with about 40 test cases, between 0 and 2 will fail on any given cargo test run; with this minimal codebase, 20-30 do.) I will try to make this minimal example look more like the codebase that produces the error which I'm reporting, and if I can I'll post back.

https://github.com/TooManyBees/wiremock-concurrency/blob/main/src/main.rs#L336-L354 is where we configure our app with two wiremock wervers, which then gets used in the test function above https://github.com/TooManyBees/wiremock-concurrency/blob/main/src/main.rs#L317-L334

LukeMathWalker commented 1 year ago

"Too many open files (os error 24)" is an error that you can fix by changing your file descriptor configuration (ulimit -n 9000). By tweaking that configuration parameter, can you reliably reproduce the actual concurrency bug here?

chris13524 commented 7 months ago

I'm encountering a similar issue, and I have a simpler reproduction example: https://github.com/chris13524/wiremock-404s It seems dependent on the AsyncTestContext.

Upon further investigation I think I know the issue with my and @TooManyBees's code. If MockServer goes out of scope it will be shutdown, allowing other MockServers to start on the same port resulting in the 404. As I mentioned above, if I remove the AsyncTextContext and start the MockServers inside each function (resulting in the reference being held until the end of the test) the tests are no longer flaky.

Similarly @TooManyBees's code is dropping the mock servers at the end of the build_default_router() function: https://github.com/TooManyBees/wiremock-concurrency/blob/16775e8b5758026d34fbc045907e8cce55da7ef2/src/main.rs#L337C13-L337C30 I suspect if the code was modified to hold the MockServer reference until the end of each test case this would not be an issue.

LukeMathWalker commented 7 months ago

Thanks for diving in @chris13524! That sounds quite accurate since it uses the RAII guard pattern to determine how long to keep the port "booked".