Skip to content

Access to ConnectionPool.count should be thread safe #318

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

Closed
ya-pulser opened this issue Nov 24, 2020 · 6 comments · Fixed by #319
Closed

Access to ConnectionPool.count should be thread safe #318

ya-pulser opened this issue Nov 24, 2020 · 6 comments · Fixed by #319
Assignees

Comments

@ya-pulser
Copy link
Contributor

ya-pulser commented Nov 24, 2020

ConnectionPool has a collection of providers registered with the pool and access to the collection is guarded by ConnectionPool.lock.

Size of the collection is exposed through count accessor without guarding by the lock.
https://github.com/swift-server/async-http-client/blob/1.2.2/Sources/AsyncHTTPClient/ConnectionPool.swift#L112

That leads to warnings in thread safety analyser.

To reproduce run with thread safety analyzer:

docker run --rm -v $(pwd):/src -w /src swift:5.3.1 swift test -c release --sanitize=thread --enable-test-discovery

Analysis of output shows that this count accessor is main source of thread safety warnings.

@ya-pulser
Copy link
Contributor Author

PR #319 as a proposed solution

@Lukasa
Copy link
Collaborator

Lukasa commented Jan 11, 2021

I believe we’re done here: see #319 (comment) for more reasoning.

@Lukasa
Copy link
Collaborator

Lukasa commented Jan 11, 2021

@weissi has a good reference for a TSAN issue in 5.3 that manifests this way.

@weissi
Copy link
Contributor

weissi commented Jul 7, 2021

@fabianfett I think we can close this?

@fabianfett
Copy link
Member

I'll add it to the http/2 milestone and close it as soon as the connection pool has landed.

@fabianfett fabianfett added this to the HTTP/2 support milestone Jul 7, 2021
@fabianfett fabianfett self-assigned this Jul 7, 2021
@fabianfett
Copy link
Member

Fixed in #319 and removed from an active code path in #427.

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 a pull request may close this issue.

4 participants