flaky test: `testConnectionPoolGrowsToMaxConcurrentConnections`

[karwa]

There are a couple of issues with testConnectionPoolGrowsToMaxConcurrentConnections:

• The maxConnections variable isn't passed down to the ConnectionPool constructor. The expected value 8 happens to match the default, but shouldn't this test use a non-default value, to check that the parameter works?

• It checks:

  XCTAssertEqual(httpBin.createdConnections, maxConnections)
  

  But actually, httpBin.createdConnections > maxConnections is also valid. The ConnectionsCountHandler doesn't actually measure the maximum number of active connections it saw -- just the total number that were created. If I have a couple of Xcode projects open, some Safari windows each with lots of tabs, I can see 10 created connections (even though when logging, I see that the maximum concurrent connections parameter was always respected).

[fabianfett]

The ConnectionsCountHandler doesn't actually measure the maximum number of active connections it saw -- just the total number that were created.

The ConnectionsCountHandler has two properties, one that counts the created connections and one that counts the currently active connections.

But actually, httpBin.createdConnections > maxConnections is also valid.

This is true if a connection failed, and needs to be recreated, which shouldn't happen in local test context. We spin up that server for the test and remove it directly after the test. Of course accessing the server while in the test will make the test fail. However I'm not sure if I would count this as flaky.

Nonetheless I'm open to a better suggestions on how to test the maximum concurrent connections in an Integration test.

[karwa]

Hmm, well I saw it happen quite a lot. I plumbed the maxConnections variable to the ConnectionPool initializer so I could tweak it, and from 0-4, everything was fine. At 5 max connections, I started seeing failures, but I closed some other Apps, it failed 3 more times, then on the 4th time it started working as my Mac recovered.

I think it would be worth adding a maxActiveConnections property to the count handler, but it's not trivial to do with NIOAtomics. I don't have the time to work on this right now, but figured it was worth reporting anyway.

[fabianfett]

I think it would be worth adding a maxActiveConnections property to the count handler, but it's not trivial to do with NIOAtomics.

I think adding a maxActiveConnections is a good idea. However I think we can reach for a lock here, without any issues.

At 5 max connections, I started seeing failures, but I closed some other Apps, it failed 3 more times, then on the 4th time it started working as my Mac recovered.

Interesting never observed this before.

Anyway, I think we should do two things:

1. Explicitly setting concurrentHTTP1ConnectionsPerHostSoftLimit in the test case
2. Adding a maxActiveConnections to ConnectionsCountHandler and using a lock inside the ConnectionsCountHandler instead of the NIOAtomics.