Skip to content

Commit 30aaba9

Browse files
VampireVampire
authored
fix(server): properly handle the lifecycle of ktor-client (#1913)
Fixes #1904 When using `ktor-client` you have to manage its lifecycle. That means, if you create a `HttpClientEngine` manually, you have to also `close()` it if it is no longer needed to free resources. If you create the `HttpClient` with an `HttpClientEngineFactory` instead, `ktor-client` is responsible to manage its lifecycle. And if you create an `HttpClient`, you also have to `close()` it if it is no longer needed to free resources, and also that the engine is closed if it is managed by `ktor-client`. Before this PR engines were created manually but never closed and clients were created but never closed. This lead to the problem that requests to the GitHub API became slower and slower and eventually fail with GitHub closing or rejecting the connections. From evidence I guess that the GitHub servers have a limit of 200 concurrent connections for a given IP and when we reached that limit over time because not closing the `ktor-client` objects, requests failed. With this PR we switch to give an engine factory to the client so that `ktor-client` manages its lifecycle and properly close the http client if it is no longer needed. For the on-demand delayed date retrievals for versions, we now also create a fresh client that is then closed after the request.
1 parent 25c1bdd commit 30aaba9

File tree

2 files changed

+108
-99
lines changed
  • shared-internal/src
    • main/kotlin/io/github/typesafegithub/workflows/shared/internal
    • test/kotlin/io/github/typesafegithub/workflows/shared/internal/model

2 files changed

+108
-99
lines changed

‎shared-internal/src/main/kotlin/io/github/typesafegithub/workflows/shared/internal/GithubApi.kt

Lines changed: 19 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,7 @@ import io.github.oshai.kotlinlogging.KotlinLogging.logger
77
import io.github.typesafegithub.workflows.shared.internal.model.Version
88
import io.ktor.client.HttpClient
99
import io.ktor.client.call.body
10-
import io.ktor.client.engine.HttpClientEngine
10+
import io.ktor.client.engine.HttpClientEngineFactory
1111
import io.ktor.client.engine.cio.CIO
1212
import io.ktor.client.plugins.contentnegotiation.ContentNegotiation
1313
import io.ktor.client.plugins.logging.LogLevel.ALL
@@ -28,32 +28,35 @@ suspend fun fetchAvailableVersions(
2828
owner: String,
2929
name: String,
3030
githubAuthToken: String?,
31-
httpClientEngine: HttpClientEngine = CIO.create(),
31+
httpClientEngineFactory: HttpClientEngineFactory<*> = CIO,
3232
): Either<String, List<Version>> =
3333
either {
34-
val httpClient = buildHttpClient(engine = httpClientEngine)
35-
return listOf(
36-
apiTagsUrl(owner = owner, name = name),
37-
apiBranchesUrl(owner = owner, name = name),
38-
).flatMap { url -> fetchGithubRefs(url, githubAuthToken, httpClient).bind() }
39-
.versions(githubAuthToken, httpClient)
34+
buildHttpClient(engineFactory = httpClientEngineFactory).use { httpClient ->
35+
return listOf(
36+
apiTagsUrl(owner = owner, name = name),
37+
apiBranchesUrl(owner = owner, name = name),
38+
).flatMap { url -> fetchGithubRefs(url, githubAuthToken, httpClient).bind() }
39+
.versions(githubAuthToken, httpClientEngineFactory)
40+
}
4041
}
4142

4243
private fun List<GithubRef>.versions(
4344
githubAuthToken: String?,
44-
httpClient: HttpClient,
45+
httpClientEngineFactory: HttpClientEngineFactory<*>,
4546
): Either<String, List<Version>> =
4647
either {
4748
this@versions.map { githubRef ->
4849
val version = githubRef.ref.substringAfterLast("/")
4950
Version(version) {
5051
val response =
51-
httpClient
52-
.get(urlString = githubRef.`object`.url) {
53-
if (githubAuthToken != null) {
54-
bearerAuth(githubAuthToken)
52+
buildHttpClient(engineFactory = httpClientEngineFactory).use { httpClient ->
53+
httpClient
54+
.get(urlString = githubRef.`object`.url) {
55+
if (githubAuthToken != null) {
56+
bearerAuth(githubAuthToken)
57+
}
5558
}
56-
}
59+
}
5760
val releaseDate =
5861
when (githubRef.`object`.type) {
5962
"tag" -> response.body<Tag>().tagger
@@ -122,8 +125,8 @@ private data class Person(
122125
val date: String,
123126
)
124127

125-
private fun buildHttpClient(engine: HttpClientEngine) =
126-
HttpClient(engine) {
128+
private fun buildHttpClient(engineFactory: HttpClientEngineFactory<*>) =
129+
HttpClient(engineFactory) {
127130
val klogger = logger
128131
install(Logging) {
129132
logger =

‎shared-internal/src/test/kotlin/io/github/typesafegithub/workflows/shared/internal/model/GithubApiTest.kt

Lines changed: 89 additions & 83 deletions
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,9 @@ import arrow.core.right
55
import io.github.typesafegithub.workflows.shared.internal.fetchAvailableVersions
66
import io.kotest.core.spec.style.FunSpec
77
import io.kotest.matchers.shouldBe
8+
import io.ktor.client.engine.HttpClientEngineFactory
89
import io.ktor.client.engine.mock.MockEngine
10+
import io.ktor.client.engine.mock.MockEngineConfig
911
import io.ktor.client.engine.mock.respond
1012
import io.ktor.http.HttpHeaders
1113
import io.ktor.http.HttpStatusCode
@@ -17,80 +19,81 @@ class GithubApiTest :
1719
FunSpec({
1820
test("branches with major versions and tags with other versions") {
1921
// Given
20-
val mockEngine =
21-
MockEngine { request ->
22-
if ("matching-refs/tags" in request.url.fullPath) {
23-
respond(
24-
// language=json
25-
content =
26-
ByteReadChannel(
27-
"""
28-
[
29-
{
30-
"ref":"refs/tags/v1.0.0",
31-
"node_id":"MDM6UmVmMTk3ODE0NjI5OnJlZnMvdGFncy92MQ==",
32-
"url":"https://api.github.com/repos/some-owner/some-name/git/refs/tags/v1",
33-
"object": {
34-
"sha":"544eadc6bf3d226fd7a7a9f0dc5b5bf7ca0675b9",
35-
"type":"tag",
36-
"url":"https://api.github.com/repos/actions/some-name/git/tags/544eadc6bf3d226fd7a7a9f0dc5b5bf7ca0675b9"
37-
}
38-
},
39-
{
40-
"ref":"refs/tags/v1.0.1",
41-
"node_id":"MDM6UmVmMTk3ODE0NjI5OnJlZnMvdGFncy92MQ==",
42-
"url":"https://api.github.com/repos/some-owner/some-name/git/refs/tags/v1.0.1",
43-
"object": {
44-
"sha":"af513c7a016048ae468971c52ed77d9562c7c819",
45-
"type":"tag",
46-
"url":"https://api.github.com/repos/actions/some-name/git/tags/af513c7a016048ae468971c52ed77d9562c7c819"
47-
}
48-
}
49-
]
50-
""".trimIndent(),
51-
),
52-
status = HttpStatusCode.OK,
53-
headers = headersOf(HttpHeaders.ContentType, "application/json"),
54-
)
55-
} else if ("matching-refs/heads" in request.url.fullPath) {
56-
respond(
57-
// language=json
58-
content =
59-
ByteReadChannel(
60-
"""
61-
[
62-
{
63-
"ref":"refs/heads/v1",
64-
"node_id":"MDM6UmVmMTk3ODE0NjI5OnJlZnMvaGVhZHMvdm1qb3NlcGgvc2lsZW50LXJldi1wYXJzZQ==",
65-
"url":"https://api.github.com/repos/some-owner/some-name/git/refs/heads/v1",
66-
"object": {
67-
"sha":"af5130cb8882054eda385840657dcbd1e19ab8f4",
68-
"type":"commit",
69-
"url":"https://api.github.com/repos/some-owner/some-name/git/commits/af5130cb8882054eda385840657dcbd1e19ab8f4"
70-
}
71-
},
72-
{
73-
"ref":"refs/heads/v2",
74-
"node_id":"MDM6UmVmMTk3ODE0NjI5OnJlZnMvaGVhZHMvdm1qb3NlcGgvdG9vbGtpdC13aW5kb3dzLWV4ZWM=",
75-
"url":"https://api.github.com/repos/some-owner/some-name/git/refs/heads/v2",
76-
"object": {
77-
"sha":"c22ccee38a13e34cb01a103c324adb1db665821e",
78-
"type":"commit",
79-
"url":"https://api.github.com/repos/some-owner/some-name/git/commits/c22ccee38a13e34cb01a103c324adb1db665821e"
80-
}
81-
}
82-
]
83-
""".trimIndent(),
84-
),
85-
status = HttpStatusCode.OK,
86-
headers = headersOf(HttpHeaders.ContentType, "application/json"),
87-
)
88-
} else {
89-
respond(
90-
content = ByteReadChannel("The mock client wasn't prepared for this request"),
91-
status = HttpStatusCode.NotFound,
92-
)
22+
val tagsResponse =
23+
"""
24+
[
25+
{
26+
"ref":"refs/tags/v1.0.0",
27+
"node_id":"MDM6UmVmMTk3ODE0NjI5OnJlZnMvdGFncy92MQ==",
28+
"url":"https://api.github.com/repos/some-owner/some-name/git/refs/tags/v1",
29+
"object": {
30+
"sha":"544eadc6bf3d226fd7a7a9f0dc5b5bf7ca0675b9",
31+
"type":"tag",
32+
"url":"https://api.github.com/repos/actions/some-name/git/tags/544eadc6bf3d226fd7a7a9f0dc5b5bf7ca0675b9"
33+
}
34+
},
35+
{
36+
"ref":"refs/tags/v1.0.1",
37+
"node_id":"MDM6UmVmMTk3ODE0NjI5OnJlZnMvdGFncy92MQ==",
38+
"url":"https://api.github.com/repos/some-owner/some-name/git/refs/tags/v1.0.1",
39+
"object": {
40+
"sha":"af513c7a016048ae468971c52ed77d9562c7c819",
41+
"type":"tag",
42+
"url":"https://api.github.com/repos/actions/some-name/git/tags/af513c7a016048ae468971c52ed77d9562c7c819"
43+
}
9344
}
45+
]
46+
""".trimIndent()
47+
val headsResponse =
48+
"""
49+
[
50+
{
51+
"ref":"refs/heads/v1",
52+
"node_id":"MDM6UmVmMTk3ODE0NjI5OnJlZnMvaGVhZHMvdm1qb3NlcGgvc2lsZW50LXJldi1wYXJzZQ==",
53+
"url":"https://api.github.com/repos/some-owner/some-name/git/refs/heads/v1",
54+
"object": {
55+
"sha":"af5130cb8882054eda385840657dcbd1e19ab8f4",
56+
"type":"commit",
57+
"url":"https://api.github.com/repos/some-owner/some-name/git/commits/af5130cb8882054eda385840657dcbd1e19ab8f4"
58+
}
59+
},
60+
{
61+
"ref":"refs/heads/v2",
62+
"node_id":"MDM6UmVmMTk3ODE0NjI5OnJlZnMvaGVhZHMvdm1qb3NlcGgvdG9vbGtpdC13aW5kb3dzLWV4ZWM=",
63+
"url":"https://api.github.com/repos/some-owner/some-name/git/refs/heads/v2",
64+
"object": {
65+
"sha":"c22ccee38a13e34cb01a103c324adb1db665821e",
66+
"type":"commit",
67+
"url":"https://api.github.com/repos/some-owner/some-name/git/commits/c22ccee38a13e34cb01a103c324adb1db665821e"
68+
}
69+
}
70+
]
71+
""".trimIndent()
72+
val mockEngineFactory =
73+
object : HttpClientEngineFactory<MockEngineConfig> {
74+
override fun create(block: MockEngineConfig.() -> Unit) =
75+
MockEngine { request ->
76+
if ("matching-refs/tags" in request.url.fullPath) {
77+
respond(
78+
// language=json
79+
content = ByteReadChannel(tagsResponse),
80+
status = HttpStatusCode.OK,
81+
headers = headersOf(HttpHeaders.ContentType, "application/json"),
82+
)
83+
} else if ("matching-refs/heads" in request.url.fullPath) {
84+
respond(
85+
// language=json
86+
content = ByteReadChannel(headsResponse),
87+
status = HttpStatusCode.OK,
88+
headers = headersOf(HttpHeaders.ContentType, "application/json"),
89+
)
90+
} else {
91+
respond(
92+
content = ByteReadChannel("The mock client wasn't prepared for this request"),
93+
status = HttpStatusCode.NotFound,
94+
)
95+
}
96+
}
9497
}
9598

9699
// When
@@ -99,7 +102,7 @@ class GithubApiTest :
99102
owner = "some-owner",
100103
name = "some-name",
101104
githubAuthToken = "token",
102-
httpClientEngine = mockEngine,
105+
httpClientEngineFactory = mockEngineFactory,
103106
)
104107

105108
// Then
@@ -114,14 +117,17 @@ class GithubApiTest :
114117

115118
test("error occurs when fetching branches and tags") {
116119
// Given
117-
val mockEngine =
118-
MockEngine { request ->
119-
respond(
120-
// language=json
121-
content = ByteReadChannel("""{"message": "There was a problem!"}"""),
122-
status = HttpStatusCode.Forbidden,
123-
headers = headersOf(HttpHeaders.ContentType, "application/json"),
124-
)
120+
val mockEngineFactory =
121+
object : HttpClientEngineFactory<MockEngineConfig> {
122+
override fun create(block: MockEngineConfig.() -> Unit) =
123+
MockEngine { request ->
124+
respond(
125+
// language=json
126+
content = ByteReadChannel("""{"message": "There was a problem!"}"""),
127+
status = HttpStatusCode.Forbidden,
128+
headers = headersOf(HttpHeaders.ContentType, "application/json"),
129+
)
130+
}
125131
}
126132

127133
// When
@@ -130,7 +136,7 @@ class GithubApiTest :
130136
owner = "some-owner",
131137
name = "some-name",
132138
githubAuthToken = "token",
133-
httpClientEngine = mockEngine,
139+
httpClientEngineFactory = mockEngineFactory,
134140
)
135141

136142
// Then

0 commit comments

Comments
 (0)