Skip to content

Reach target parallelism in .limitedParallelism of failing dispatchers #4330

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

Merged
merged 2 commits into from
Jan 15, 2025

Conversation

dkhalanskyjb
Copy link
Collaborator

A tiny self-contained piece of #4277

@dkhalanskyjb dkhalanskyjb requested a review from qwwdfsad January 14, 2025 13:56
dispatch does succeed.
If we don't decrement the counter, it will be impossible to ever reach the target parallelism again. */
runningWorkers.decrementAndGet()
throw e
Copy link
Contributor

@JakeWharton JakeWharton Jan 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems to basically be a finally block. Any reason not to use that, instead of catch+rethrow? Same holds true below.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reason to use catch/throw is that we don't want to deallocate the worker if startWorker finishes normally, as this means the worker is actually started. The same holds true below: if return is used, this means we've already deallocated the worker and don't need to do that again.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, right right. Thanks for clarifying.

@dkhalanskyjb dkhalanskyjb merged commit ec0d2c6 into develop Jan 15, 2025
1 check passed
@dkhalanskyjb dkhalanskyjb deleted the dk-fix-leaking-limitedParallelism-jobs branch January 15, 2025 08:53
svc-squareup-copybara pushed a commit to cashapp/misk that referenced this pull request Apr 8, 2025
| Package | Type | Package file | Manager | Update | Change |
|---|---|---|---|---|---|
|
[org.jetbrains.kotlinx:kotlinx-coroutines-test](https://github.com/Kotlin/kotlinx.coroutines)
| dependencies | misk/gradle/libs.versions.toml | gradle | patch |
`1.10.1` -> `1.10.2` |
|
[org.jetbrains.kotlinx:kotlinx-coroutines-slf4j](https://github.com/Kotlin/kotlinx.coroutines)
| dependencies | misk/gradle/libs.versions.toml | gradle | patch |
`1.10.1` -> `1.10.2` |
|
[org.jetbrains.kotlinx:kotlinx-coroutines-core](https://github.com/Kotlin/kotlinx.coroutines)
| dependencies | misk/gradle/libs.versions.toml | gradle | patch |
`1.10.1` -> `1.10.2` |
| [software.amazon.awssdk:sdk-core](https://aws.amazon.com/sdkforjava) |
dependencies | misk/gradle/libs.versions.toml | gradle | patch |
`2.31.16` -> `2.31.17` |
| [software.amazon.awssdk:sqs](https://aws.amazon.com/sdkforjava) |
dependencies | misk/gradle/libs.versions.toml | gradle | patch |
`2.31.16` -> `2.31.17` |
|
[software.amazon.awssdk:dynamodb-enhanced](https://aws.amazon.com/sdkforjava)
| dependencies | misk/gradle/libs.versions.toml | gradle | patch |
`2.31.16` -> `2.31.17` |
| [software.amazon.awssdk:dynamodb](https://aws.amazon.com/sdkforjava) |
dependencies | misk/gradle/libs.versions.toml | gradle | patch |
`2.31.16` -> `2.31.17` |
| [software.amazon.awssdk:aws-core](https://aws.amazon.com/sdkforjava) |
dependencies | misk/gradle/libs.versions.toml | gradle | patch |
`2.31.16` -> `2.31.17` |
| [software.amazon.awssdk:bom](https://aws.amazon.com/sdkforjava) |
dependencies | misk/gradle/libs.versions.toml | gradle | patch |
`2.31.16` -> `2.31.17` |
| [software.amazon.awssdk:auth](https://aws.amazon.com/sdkforjava) |
dependencies | misk/gradle/libs.versions.toml | gradle | patch |
`2.31.16` -> `2.31.17` |

---

### Release Notes

<details>
<summary>Kotlin/kotlinx.coroutines
(org.jetbrains.kotlinx:kotlinx-coroutines-test)</summary>

###
[`v1.10.2`](https://github.com/Kotlin/kotlinx.coroutines/blob/HEAD/CHANGES.md#Version-1102)

[Compare
Source](Kotlin/kotlinx.coroutines@1.10.1...1.10.2)

- Fixed the `kotlinx-coroutines-debug` JAR file including the
`module-info.class` file twice, resulting in failures in various tooling
([#&#8203;4314](Kotlin/kotlinx.coroutines#4314)).
Thanks, [@&#8203;RyuNen344](https://github.com/RyuNen344)!
- Fixed `Flow.stateIn` hanging when the scope is cancelled in advance or
the flow is empty
([#&#8203;4322](Kotlin/kotlinx.coroutines#4322)).
Thanks, [@&#8203;francescotescari](https://github.com/francescotescari)!
- Improved handling of dispatcher failures in `.limitedParallelism`
([#&#8203;4330](Kotlin/kotlinx.coroutines#4330))
and during flow collection
([#&#8203;4272](Kotlin/kotlinx.coroutines#4272)).
- Fixed `runBlocking` failing to run its coroutine to completion in some
cases if its JVM thread got interrupted
([#&#8203;4399](Kotlin/kotlinx.coroutines#4399)).
-   Small tweaks, fixes, and documentation improvements.

</details>

---

### Configuration

📅 **Schedule**: Branch creation - "after 6pm every weekday,before 2am
every weekday" in timezone Australia/Melbourne, Automerge - At any time
(no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you
are satisfied.

♻ **Rebasing**: Never, or you tick the rebase/retry checkbox.

👻 **Immortal**: This PR will be recreated if closed unmerged. Get
[config help](https://github.com/renovatebot/renovate/discussions) if
that's undesired.

---

- [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check
this box

---

This PR has been generated by [Renovate
Bot](https://github.com/renovatebot/renovate).

GitOrigin-RevId: 3449fc325ac14573dff5b670e407e99eab23757b
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 this pull request may close these issues.

3 participants