Skip to content

fix: Call Release on resource semaphore#279

Merged
kodiakhq[bot] merged 2 commits intomainfrom
fix-resource-concurrency
Oct 11, 2022
Merged

fix: Call Release on resource semaphore#279
kodiakhq[bot] merged 2 commits intomainfrom
fix-resource-concurrency

Conversation

@hermanschaaf
Copy link
Copy Markdown
Contributor

@hermanschaaf hermanschaaf commented Oct 11, 2022

We were missing a Release call on the resource_concurrency semaphore.

Copy link
Copy Markdown
Contributor

@yevgenypats yevgenypats left a comment

Choose a reason for hiding this comment

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

small nit on order

wg.Add(1)
go func() {
defer wg.Done()
defer resourcesSem.Release(1)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nit: let's keep the order the same

so it should be:

defer resourceSem.Release(1) (release the resource last because we acquired it first)
defer wg.Done() (so first we release the workgroup as it was add last)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

wg.Done() 👍

@kodiakhq kodiakhq bot merged commit 051e247 into main Oct 11, 2022
@kodiakhq kodiakhq bot deleted the fix-resource-concurrency branch October 11, 2022 11:06
hermanschaaf pushed a commit that referenced this pull request Oct 11, 2022
🤖 I have created a release *beep* *boop*
---


##
[0.13.3](v0.13.2...v0.13.3)
(2022-10-11)


### Bug Fixes

* Call Release on resource semaphore
([#279](#279))
([051e247](051e247))

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants