Always access Task’s connection and cancelled property through a lock #385
Conversation
fabianfett
force-pushed
the
ff-make-thread-safe
branch
from
e226032 to
aff64fd
Compare
|
Failure in known flaky test #371. @swift-server-bot test this please |
| let requestFuture = httpClient.execute(request: request, delegate: delegate).futureResult | ||
| let logger = Logger(label: "test-connection") | ||
|
|
||
| let clientFactory = HTTPConnectionPool.ConnectionFactory( |
There was a problem hiding this comment.
Why has this dropped down to be so low-level?
There was a problem hiding this comment.
I would really like to not expose the connection on the task in the future. In order to have control over the channel, we need to create it ourselves here.
There was a problem hiding this comment.
There is only one other test remaining that accesses the task.connection:
testWeCanActuallyExactlySetTheEventLoops
We should be able to remove task.connection from testWeCanActuallyExactlySetTheEventLoops, once the new connection pool has landed. We would test only the delegate calls, since all connection eventLoop requirements can be tested with unit tests and will be tested with unit tests.
There was a problem hiding this comment.
Ok, can we encapsulate this new code in a function that clearly explains what it does? The code here is very aware of implementation details of AHC, which suggests the test as a whole needs rewriting, but in the near term I'd be ok with just hiding the implementation details in a function.
There was a problem hiding this comment.
Yes, ignored it for now, since I think we should be able to just remove this test, once other PRs have landed. The functionality will be tested by unit tests.
| self.lock.withLock { self._cancelled } | ||
| } | ||
|
|
||
| var connection: Connection? { |
There was a problem hiding this comment.
It seems like this is only used in tests: can we mark it clearly for that purpose?
There was a problem hiding this comment.
Sorry, I wasn't clear: I meant changing the name to var _testOnly_connection
There was a problem hiding this comment.
This is used on L715, maybe make it private and add an internal _testOnly_connection per Cory's suggestion?
| @@ -702,7 +713,9 @@ extension HTTPClient { | |||
|
|
|||
| func fail<Delegate: HTTPClientResponseDelegate>(with error: Error, | |||
| delegateType: Delegate.Type) { | |||
| if let connection = self.connection { | |||
| let maybeConnection = self.lock.withLock { self._connection } | |||
There was a problem hiding this comment.
nit: why not use self.connection here?
| @@ -681,13 +689,16 @@ extension HTTPClient { | |||
|
|
|||
| @discardableResult | |||
| func setConnection(_ connection: Connection) -> Connection { | |||
There was a problem hiding this comment.
not related to this PR, but why do we also return the connection that we set here?
Motivation
connectionandcancelledare sometime accessed through a lock and sometimes not.Modifications