Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Removing Subscription consumer re-registers the consumer instead of removing #9123

Closed
oleschoenburg opened this issue Apr 13, 2022 · 3 comments · Fixed by #9139
Closed

Removing Subscription consumer re-registers the consumer instead of removing #9123

oleschoenburg opened this issue Apr 13, 2022 · 3 comments · Fixed by #9139
Assignees
@oleschoenburg
Labels
area/reliability Marks an issue as related to improving the reliability of our software (i.e. it behaves as expected) kind/bug Categorizes an issue or PR as a bug scope/broker Marks an issue or PR to appear in the broker section of the changelog severity/low Marks a bug as having little to no noticeable impact for the user version:1.3.8 version:8.1.0-alpha1 Marks an issue as being completely or in parts released in 8.1.0-alpha1 version:8.1.0 Marks an issue as being completely or in parts released in 8.1.0

Comments

@oleschoenburg
Copy link
Member

Describe the bug

zeebe/dispatcher/src/main/java/io/camunda/zeebe/dispatcher/Subscription.java

Lines 83 to 86 in 1edfe4f

@Override
public void removeConsumer(final ActorCondition consumer) {
actorConditions.registerConsumer(consumer);
}

this should obviously be a call to removeConsumer, not registerConsumer. As far as I can tell this bug has existed since at least 2018.

Removing a subscription consumer is triggered when an ActorTask is closed (successfully or due to failure).
@oleschoenburg oleschoenburg added kind/bug Categorizes an issue or PR as a bug scope/broker Marks an issue or PR to appear in the broker section of the changelog area/reliability Marks an issue as related to improving the reliability of our software (i.e. it behaves as expected) team/distributed labels Apr 13, 2022
@Zelldon
Copy link
Member

Zelldon commented Apr 13, 2022

Lol 😆

@npepinpe
Copy link
Member

Love it 😄

@npepinpe
Copy link
Member

npepinpe commented Apr 13, 2022 •
edited

Seems like an low hanging fruit, let's just go ahead and fix it (unless there's no actorConditions.removeConsumer, or worse, we actually rely on this behavior 😱)

Marked it as low severity since we never noticed it 🤷‍♂️

@npepinpe npepinpe added the severity/low Marks a bug as having little to no noticeable impact for the user label Apr 13, 2022
@oleschoenburg oleschoenburg mentioned this issue Apr 13, 2022
Merged
@oleschoenburg oleschoenburg self-assigned this Apr 14, 2022
zeebe-bors-camunda bot added a commit that referenced this issue Apr 14, 2022
@zeebe-bors-camunda @oleschoenburg
merge: #9139
dee467d 
9139: fix: remove subscription consumer instead of re-registering r=oleschoenburg a=oleschoenburg

This was probably not an issue because it was mitigated by other circumstances.

closes #9123 


Co-authored-by: Ole Schönburg <ole.schoenburg@gmail.com>
zeebe-bors-camunda bot added a commit that referenced this issue Apr 14, 2022
@zeebe-bors-camunda @oleschoenburg
merge: #9139
4f51a15 
9139: fix: remove subscription consumer instead of re-registering r=oleschoenburg a=oleschoenburg

This was probably not an issue because it was mitigated by other circumstances.

closes #9123 


Co-authored-by: Ole Schönburg <ole.schoenburg@gmail.com>
zeebe-bors-camunda bot added a commit that referenced this issue Apr 14, 2022
@zeebe-bors-camunda @oleschoenburg
merge: #9139
73a6eb3 
9139: fix: remove subscription consumer instead of re-registering r=oleschoenburg a=oleschoenburg

This was probably not an issue because it was mitigated by other circumstances.

closes #9123 


Co-authored-by: Ole Schönburg <ole.schoenburg@gmail.com>
zeebe-bors-camunda bot added a commit that referenced this issue Apr 14, 2022
@zeebe-bors-camunda @oleschoenburg @dependabot
merge: #9139 #9149 #9150
7483104 
9139: fix: remove subscription consumer instead of re-registering r=oleschoenburg a=oleschoenburg

This was probably not an issue because it was mitigated by other circumstances.

closes #9123 


9149: deps(maven): bump netty-bom from 4.1.75.Final to 4.1.76.Final r=npepinpe a=dependabot[bot]

Bumps [netty-bom](https://github.com/netty/netty) from 4.1.75.Final to 4.1.76.Final.
<details>
<summary>Commits</summary>
<ul>
<li><a href="https://github.com/netty/netty/commit/701263610a72725c9d32b4022f1d51b1344890d1"><code>7012636</code></a> [maven-release-plugin] prepare release netty-4.1.76.Final</li>
<li><a href="https://github.com/netty/netty/commit/1ecc053e70d57a8da70bf23bf5293b2072337b09"><code>1ecc053</code></a> Log specific exception to debug ParameterizedSslHandlerTest (<a href="https://github-redirect.dependabot.com/netty/netty/issues/12290">#12290</a>)</li>
<li><a href="https://github.com/netty/netty/commit/cf8b7170505c2329b34ff1f8c0b76977a4d7c2c2"><code>cf8b717</code></a> Cope with misbehaving Http2Headers implementations (<a href="https://github-redirect.dependabot.com/netty/netty/issues/12289">#12289</a>)</li>
<li><a href="https://github.com/netty/netty/commit/a9cac5ee0b921deaa51758f038f9316ca821616a"><code>a9cac5e</code></a> Share code and improve logging (<a href="https://github-redirect.dependabot.com/netty/netty/issues/12282">#12282</a>)</li>
<li><a href="https://github.com/netty/netty/commit/005a6a4e7b3632d6719640eac621690d3268fd1f"><code>005a6a4</code></a> Correctly encode result even if no timeout is used (<a href="https://github-redirect.dependabot.com/netty/netty/issues/12283">#12283</a>)</li>
<li><a href="https://github.com/netty/netty/commit/5e23212172a761c416ec682c55f462852c381e92"><code>5e23212</code></a> Break loop early if possible (<a href="https://github-redirect.dependabot.com/netty/netty/issues/12254">#12254</a>)</li>
<li><a href="https://github.com/netty/netty/commit/960121db545f2d8aec7a87da8c54a9645fde266c"><code>960121d</code></a> Allow explicit choice of internet family (<a href="https://github-redirect.dependabot.com/netty/netty/issues/12270">#12270</a>)</li>
<li><a href="https://github.com/netty/netty/commit/afbf8d26b17548896b55d4edbcdcc4f8bb625a7d"><code>afbf8d2</code></a> Upgrade to <code>Brotli4j</code> 1.7.1 and use new <code>ByteBuf</code> API (<a href="https://github-redirect.dependabot.com/netty/netty/issues/12264">#12264</a>)</li>
<li><a href="https://github.com/netty/netty/commit/34ae67a5b1fb1f119c1ed509f576c205d1e7fd78"><code>34ae67a</code></a> Bump Log4j2 to 2.17.2 (<a href="https://github-redirect.dependabot.com/netty/netty/issues/12261">#12261</a>)</li>
<li><a href="https://github.com/netty/netty/commit/19d74affb0bc88118d64737df10569ac41ae1804"><code>19d74af</code></a> Automatically use boringssl-static when compiling on Mac M1 (<a href="https://github-redirect.dependabot.com/netty/netty/issues/12256">#12256</a>)</li>
<li>Additional commits viewable in <a href="https://github.com/netty/netty/compare/netty-4.1.75.Final...netty-4.1.76.Final">compare view</a></li>
</ul>
</details>
<br />


[![Dependabot compatibility score](https://dependabot-badges.githubapp.com/badges/compatibility_score?dependency-name=io.netty:netty-bom&package-manager=maven&previous-version=4.1.75.Final&new-version=4.1.76.Final)](https://docs.github.com/en/github/managing-security-vulnerabilities/about-dependabot-security-updates#about-compatibility-scores)

Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting ``@dependabot` rebase`.

[//]: # (dependabot-automerge-start)
[//]: # (dependabot-automerge-end)

---

<details>
<summary>Dependabot commands and options</summary>
<br />

You can trigger Dependabot actions by commenting on this PR:
- ``@dependabot` rebase` will rebase this PR
- ``@dependabot` recreate` will recreate this PR, overwriting any edits that have been made to it
- ``@dependabot` merge` will merge this PR after your CI passes on it
- ``@dependabot` squash and merge` will squash and merge this PR after your CI passes on it
- ``@dependabot` cancel merge` will cancel a previously requested merge and block automerging
- ``@dependabot` reopen` will reopen this PR if it is closed
- ``@dependabot` close` will close this PR and stop Dependabot recreating it. You can achieve the same result by closing it manually
- ``@dependabot` ignore this major version` will close this PR and stop Dependabot creating any more for this major version (unless you reopen the PR or upgrade to it yourself)
- ``@dependabot` ignore this minor version` will close this PR and stop Dependabot creating any more for this minor version (unless you reopen the PR or upgrade to it yourself)
- ``@dependabot` ignore this dependency` will close this PR and stop Dependabot creating any more for this dependency (unless you reopen the PR or upgrade to it yourself)


</details>

9150: deps(maven): bump proto-google-common-protos from 2.8.2 to 2.8.3 r=npepinpe a=dependabot[bot]

Bumps [proto-google-common-protos](https://github.com/googleapis/java-iam) from 2.8.2 to 2.8.3.
<details>
<summary>Commits</summary>
<ul>
<li>See full diff in <a href="https://github.com/googleapis/java-iam/commits">compare view</a></li>
</ul>
</details>
<br />


[![Dependabot compatibility score](https://dependabot-badges.githubapp.com/badges/compatibility_score?dependency-name=com.google.api.grpc:proto-google-common-protos&package-manager=maven&previous-version=2.8.2&new-version=2.8.3)](https://docs.github.com/en/github/managing-security-vulnerabilities/about-dependabot-security-updates#about-compatibility-scores)

Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting ``@dependabot` rebase`.

[//]: # (dependabot-automerge-start)
[//]: # (dependabot-automerge-end)

---

<details>
<summary>Dependabot commands and options</summary>
<br />

You can trigger Dependabot actions by commenting on this PR:
- ``@dependabot` rebase` will rebase this PR
- ``@dependabot` recreate` will recreate this PR, overwriting any edits that have been made to it
- ``@dependabot` merge` will merge this PR after your CI passes on it
- ``@dependabot` squash and merge` will squash and merge this PR after your CI passes on it
- ``@dependabot` cancel merge` will cancel a previously requested merge and block automerging
- ``@dependabot` reopen` will reopen this PR if it is closed
- ``@dependabot` close` will close this PR and stop Dependabot recreating it. You can achieve the same result by closing it manually
- ``@dependabot` ignore this major version` will close this PR and stop Dependabot creating any more for this major version (unless you reopen the PR or upgrade to it yourself)
- ``@dependabot` ignore this minor version` will close this PR and stop Dependabot creating any more for this minor version (unless you reopen the PR or upgrade to it yourself)
- ``@dependabot` ignore this dependency` will close this PR and stop Dependabot creating any more for this dependency (unless you reopen the PR or upgrade to it yourself)


</details>

Co-authored-by: Ole Schönburg <ole.schoenburg@gmail.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
This was referenced Apr 14, 2022
Merged
Merged
zeebe-bors-camunda bot added a commit that referenced this issue Apr 14, 2022
@zeebe-bors-camunda @oleschoenburg
merge: #9151
c7e2095 
9151: [Backport stable/1.3] fix: remove subscription consumer instead of re-registering r=oleschoenburg a=github-actions[bot]

# Description
Backport of #9139 to `stable/1.3`.

relates to #9123

Co-authored-by: Ole Schönburg <ole.schoenburg@gmail.com>
zeebe-bors-camunda bot added a commit that referenced this issue Apr 14, 2022
@zeebe-bors-camunda @oleschoenburg
merge: #9152
ad802a9 
9152: [Backport stable/8.0] fix: remove subscription consumer instead of re-registering r=oleschoenburg a=github-actions[bot]

# Description
Backport of #9139 to `stable/8.0`.

relates to #9123

Co-authored-by: Ole Schönburg <ole.schoenburg@gmail.com>
@deepthidevaki deepthidevaki added the version:8.1.0-alpha1 Marks an issue as being completely or in parts released in 8.1.0-alpha1 label May 3, 2022
@Zelldon Zelldon added the version:8.1.0 Marks an issue as being completely or in parts released in 8.1.0 label Oct 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@oleschoenburg oleschoenburg

Labels
area/reliability Marks an issue as related to improving the reliability of our software (i.e. it behaves as expected) kind/bug Categorizes an issue or PR as a bug scope/broker Marks an issue or PR to appear in the broker section of the changelog severity/low Marks a bug as having little to no noticeable impact for the user version:1.3.8 version:8.1.0-alpha1 Marks an issue as being completely or in parts released in 8.1.0-alpha1 version:8.1.0 Marks an issue as being completely or in parts released in 8.1.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

fix: remove subscription consumer instead of re-registering camunda/zeebe
4 participants
@npepinpe @oleschoenburg @deepthidevaki @Zelldon