You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by "shibd (via GitHub)" <gi...@apache.org> on 2025/03/20 07:31:46 UTC
[PR] [fix][client] Consumer leak when thread.Interrupted is called before subscribed [pulsar]
shibd opened a new pull request, #24100:
URL: https://github.com/apache/pulsar/pull/24100
### Motivation
The major issue is customer has a thread try to subscribe(create a consumer) to a topic.
But the thread get interrupted before getting a response for the consumer creation by their scheduling service.
- Thread A - call subscribe() method
- Thread A - blocking on waiting for the response of the consumer creation
- The user framework interrupted `Thread A` for aborting a job.
- Cannot close the consumer since user will get InterruptedException and can't get consumer object
### Modifications
- Added a variable named `interruptedBeforeConsumerCreation`. When this variable is true, the consumer needs to be closed.
### Verifying this change
- You can run `testInterruptedWhenCreateConsumer ` to reproduce this issue.
### Documentation
<!-- DO NOT REMOVE THIS SECTION. CHECK THE PROPER BOX ONLY. -->
- [ ] `doc` <!-- Your PR contains doc changes. -->
- [ ] `doc-required` <!-- Your PR changes impact docs and you will update later -->
- [x] `doc-not-needed` <!-- Your PR changes do not impact docs -->
- [ ] `doc-complete` <!-- Docs have been already added -->
### Matching PR in forked repository
PR in forked repository: <!-- ENTER URL HERE -->
<!--
After opening this PR, the build in apache/pulsar will fail and instructions will
be provided for opening a PR in the PR author's forked repository.
apache/pulsar pull requests should be first tested in your own fork since the
apache/pulsar CI based on GitHub Actions has constrained resources and quota.
GitHub Actions provides separate quota for pull requests that are executed in
a forked repository.
The tests will be run in the forked repository until all PR review comments have
been handled, the tests pass and the PR is approved by a reviewer.
-->
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [fix][client] Consumer leak when thread.Interrupted is called before subscribed [pulsar]
Posted by "shibd (via GitHub)" <gi...@apache.org>.
shibd commented on PR #24100:
URL: https://github.com/apache/pulsar/pull/24100#issuecomment-2741992850
hi, @lhotari , I respect all your opinions, but I really didn't understand what you meant.
Could you comment directly on the code about what needs to be changed? Or could you show the test you want directly in the code?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [fix][client] Consumer leak when thread.Interrupted is called before subscribed [pulsar]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #24100:
URL: https://github.com/apache/pulsar/pull/24100#issuecomment-2739935558
## [Codecov](https://app.codecov.io/gh/apache/pulsar/pull/24100?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
Attention: Patch coverage is `88.88889%` with `2 lines` in your changes missing coverage. Please review.
> Project coverage is 74.24%. Comparing base [(`bbc6224`)](https://app.codecov.io/gh/apache/pulsar/commit/bbc62245c5ddba1de4b1e7cee4ab49334bc36277?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) to head [(`e7922a3`)](https://app.codecov.io/gh/apache/pulsar/commit/e7922a3658d05210e845348f023321f353e0263f?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
> Report is 979 commits behind head on master.
| [Files with missing lines](https://app.codecov.io/gh/apache/pulsar/pull/24100?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [...apache/pulsar/client/impl/ConsumerBuilderImpl.java](https://app.codecov.io/gh/apache/pulsar/pull/24100?src=pr&el=tree&filepath=pulsar-client%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpulsar%2Fclient%2Fimpl%2FConsumerBuilderImpl.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cHVsc2FyLWNsaWVudC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL2NsaWVudC9pbXBsL0NvbnN1bWVyQnVpbGRlckltcGwuamF2YQ==) | 88.88% | [2 Missing :warning: ](https://app.codecov.io/gh/apache/pulsar/pull/24100?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
<details><summary>Additional details and impacted files</summary>
[](https://app.codecov.io/gh/apache/pulsar/pull/24100?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
```diff
@@ Coverage Diff @@
## master #24100 +/- ##
============================================
+ Coverage 73.57% 74.24% +0.66%
+ Complexity 32624 32452 -172
============================================
Files 1877 1863 -14
Lines 139502 144327 +4825
Branches 15299 16458 +1159
============================================
+ Hits 102638 107150 +4512
+ Misses 28908 28710 -198
- Partials 7956 8467 +511
```
| [Flag](https://app.codecov.io/gh/apache/pulsar/pull/24100/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [inttests](https://app.codecov.io/gh/apache/pulsar/pull/24100/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `26.71% <50.00%> (+2.12%)` | :arrow_up: |
| [systests](https://app.codecov.io/gh/apache/pulsar/pull/24100/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `23.19% <50.00%> (-1.14%)` | :arrow_down: |
| [unittests](https://app.codecov.io/gh/apache/pulsar/pull/24100/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `73.74% <88.88%> (+0.90%)` | :arrow_up: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Files with missing lines](https://app.codecov.io/gh/apache/pulsar/pull/24100?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [...apache/pulsar/client/impl/ConsumerBuilderImpl.java](https://app.codecov.io/gh/apache/pulsar/pull/24100?src=pr&el=tree&filepath=pulsar-client%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpulsar%2Fclient%2Fimpl%2FConsumerBuilderImpl.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cHVsc2FyLWNsaWVudC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL2NsaWVudC9pbXBsL0NvbnN1bWVyQnVpbGRlckltcGwuamF2YQ==) | `86.87% <88.88%> (-0.07%)` | :arrow_down: |
... and [1066 files with indirect coverage changes](https://app.codecov.io/gh/apache/pulsar/pull/24100/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
</details>
<details><summary>🚀 New features to boost your workflow: </summary>
- ❄ [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- 📦 [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [fix][client] Consumer leak when thread.Interrupted is called before subscribed [pulsar]
Posted by "codelipenghui (via GitHub)" <gi...@apache.org>.
codelipenghui commented on code in PR #24100:
URL: https://github.com/apache/pulsar/pull/24100#discussion_r2006699969
##########
pulsar-client/src/main/java/org/apache/pulsar/client/impl/ConsumerBuilderImpl.java:
##########
@@ -98,8 +101,31 @@ public ConsumerBuilder<T> clone() {
@Override
public Consumer<T> subscribe() throws PulsarClientException {
+ CompletableFuture<Consumer<T>> future = new CompletableFuture<>();
try {
- return subscribeAsync().get();
+ subscribeAsync().whenComplete((c, e) -> {
+ if (e != null) {
+ // If the subscription fails, there is no need to close the consumer here,
+ // as it will be handled in the subscribeAsync method.
+ future.completeExceptionally(e);
+ return;
+ }
+ if (interruptedBeforeConsumerCreation) {
+ c.closeAsync().exceptionally(closeEx -> {
+ log.error("Failed to close consumer after interruption", closeEx.getCause());
+ return null;
+ });
+ future.completeExceptionally(new PulsarClientException(
+ "Subscription was interrupted before the consumer could be fully created"));
+ } else {
+ future.complete(c);
+ }
+ });
+ return future.get();
+ } catch (InterruptedException e) {
+ interruptedBeforeConsumerCreation = true;
+ Thread.currentThread().interrupt();
+ throw PulsarClientException.unwrap(e);
Review Comment:
```suggestion
subscribeAsync().whenComplete((c, e) -> {
if (e != null) {
// If the subscription fails, there is no need to close the consumer here,
// as it will be handled in the subscribeAsync method.
future.completeExceptionally(e);
return;
}
if (interruptedBeforeConsumerCreation) {
c.closeAsync().whenComplete((__, closeEx) -> {
if (closeEx != null) {
log.error("Failed to close consumer after interruption", closeEx.getCause());
future.completeExceptionally(new PulsarClientException(interruptedException));
} else {
future.completeExceptionally(new PulsarClientException(interruptedException));
}
})
} else {
future.complete(c);
}
});
return future.get();
} catch (InterruptedException e) {
interruptedBeforeConsumerCreation = true;
interruptedException = e;
Thread.currentThread().interrupt();
```
##########
pulsar-client/src/main/java/org/apache/pulsar/client/impl/ConsumerBuilderImpl.java:
##########
@@ -98,8 +101,31 @@ public ConsumerBuilder<T> clone() {
@Override
public Consumer<T> subscribe() throws PulsarClientException {
+ CompletableFuture<Consumer<T>> future = new CompletableFuture<>();
try {
- return subscribeAsync().get();
+ subscribeAsync().whenComplete((c, e) -> {
+ if (e != null) {
+ // If the subscription fails, there is no need to close the consumer here,
+ // as it will be handled in the subscribeAsync method.
+ future.completeExceptionally(e);
+ return;
+ }
+ if (interruptedBeforeConsumerCreation) {
+ c.closeAsync().exceptionally(closeEx -> {
+ log.error("Failed to close consumer after interruption", closeEx.getCause());
+ return null;
+ });
+ future.completeExceptionally(new PulsarClientException(
+ "Subscription was interrupted before the consumer could be fully created"));
+ } else {
+ future.complete(c);
+ }
+ });
+ return future.get();
+ } catch (InterruptedException e) {
+ interruptedBeforeConsumerCreation = true;
+ Thread.currentThread().interrupt();
+ throw PulsarClientException.unwrap(e);
Review Comment:
Is it better to let the consumer get closed first and then return exception to user?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [fix][client] Consumer leak when thread.Interrupted is called before subscribed [pulsar]
Posted by "lhotari (via GitHub)" <gi...@apache.org>.
lhotari commented on PR #24100:
URL: https://github.com/apache/pulsar/pull/24100#issuecomment-2740250910
> In this case, regardless of this fix. the user wont get the consumer object, which is expected.
>
> Because the subscribe throw an exception, that's not the focus. The focus is that we need to close this consumer.
@shibd I point was that once there's a bug, a good practice would be to add a failing test case which reproduces the issue and clarifies it. In this case "the user wont get the consumer object, which is expected" could be understood in many ways. It might be clear to yourself, but not for code reviewers. That's why "failing test cases" reproducing the exact problem are always desired when it's possible to add one. Obviously for many Pulsar bugs that's really hard. In this case, it most likely isn't hard.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [fix][client] Consumer leak when thread.Interrupted is called before subscribed [pulsar]
Posted by "lhotari (via GitHub)" <gi...@apache.org>.
lhotari commented on PR #24100:
URL: https://github.com/apache/pulsar/pull/24100#issuecomment-2742571561
@shibd this is the unedited DeepSeek suggestion: https://gist.github.com/lhotari/c76c08a59c6c17b27425cea04b7fa215
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [fix][client] Consumer leak when thread.Interrupted is called before subscribed [pulsar]
Posted by "lhotari (via GitHub)" <gi...@apache.org>.
lhotari commented on PR #24100:
URL: https://github.com/apache/pulsar/pull/24100#issuecomment-2742559297
> hi, @lhotari , I respect all your opinions, but I really didn't understand what you meant.
>
> Could you comment directly on the code about what needs to be changed? Or could you show the test you want directly in the code?
Thanks @shibd.
It is simply about "Cannot close the consumer since user will get InterruptedException and in this case user can't get consumer object". I don't understand the part "and in this case user can't get consumer object" and was expecting that a test case would have clarified it.
Now, reading your comments again, it seems that "user can't get consumer object" is expected behavior and this sentence is just misleading and that's not something to be tested in the first place as you have described.
Rephrasing the sentence in the description would be a way to resolve this in that case. This isn't a blocker.
I'll check what a LLM would suggest for the PR description using the solution described in https://gist.github.com/lhotari/81f533af4b9ad515e02d96e543c4408b. It usually gives a pretty good draft and an LLM can be used to also fix grammar issues in the PR description.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [fix][client] Consumer leak when thread.Interrupted is called before subscribed [pulsar]
Posted by "shibd (via GitHub)" <gi...@apache.org>.
shibd commented on PR #24100:
URL: https://github.com/apache/pulsar/pull/24100#issuecomment-2740258068
You can run `testInterruptedWhenCreateConsumer ` to reproduce it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [fix][client] Consumer leak when thread.Interrupted is called before subscribed [pulsar]
Posted by "shibd (via GitHub)" <gi...@apache.org>.
shibd commented on code in PR #24100:
URL: https://github.com/apache/pulsar/pull/24100#discussion_r2006705755
##########
pulsar-client/src/main/java/org/apache/pulsar/client/impl/ConsumerBuilderImpl.java:
##########
@@ -98,8 +101,31 @@ public ConsumerBuilder<T> clone() {
@Override
public Consumer<T> subscribe() throws PulsarClientException {
+ CompletableFuture<Consumer<T>> future = new CompletableFuture<>();
try {
- return subscribeAsync().get();
+ subscribeAsync().whenComplete((c, e) -> {
+ if (e != null) {
+ // If the subscription fails, there is no need to close the consumer here,
+ // as it will be handled in the subscribeAsync method.
+ future.completeExceptionally(e);
+ return;
+ }
+ if (interruptedBeforeConsumerCreation) {
+ c.closeAsync().exceptionally(closeEx -> {
+ log.error("Failed to close consumer after interruption", closeEx.getCause());
+ return null;
+ });
+ future.completeExceptionally(new PulsarClientException(
+ "Subscription was interrupted before the consumer could be fully created"));
+ } else {
+ future.complete(c);
+ }
+ });
+ return future.get();
+ } catch (InterruptedException e) {
+ interruptedBeforeConsumerCreation = true;
+ Thread.currentThread().interrupt();
+ throw PulsarClientException.unwrap(e);
Review Comment:
Yes, this is better from the code side.
However, to clarify, in this case, the user won't actually get this exception(`future.completeExceptionally(new PulsarClientException(interruptedException));`) because future.get() has already thrown an InterruptedException and returned it to the user.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [fix][client] Consumer leak when thread.Interrupted is called before subscribed [pulsar]
Posted by "shibd (via GitHub)" <gi...@apache.org>.
shibd commented on PR #24100:
URL: https://github.com/apache/pulsar/pull/24100#issuecomment-2741963866
First, I clearly asserted here that when the user calls `thread.interrupt `, the `subscribe` method will throw an exception.
Isn't that enough?
https://github.com/apache/pulsar/pull/24100/files#diff-16facc169f1bbd7ab0a86d608b2aaa7ab8a578ff83c1d71d784b3df55d651559R71-R81
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [fix][client] Consumer leak when thread.Interrupted is called before subscribed [pulsar]
Posted by "shibd (via GitHub)" <gi...@apache.org>.
shibd commented on PR #24100:
URL: https://github.com/apache/pulsar/pull/24100#issuecomment-2740234792
> The "can't get consumer object" part isn't currently tested so that after fixing the issue the "consumer object can be get" (the test would clarify what this actually means).
In this case, regardless of this fix. the user wont get the consumer object, which is expected.
Because the subscribe throw an exception, that's not the focus. The focus is that we need to close this consumer.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [fix][client] Fix consumer leak when thread is interrupted before subscribe completes [pulsar]
Posted by "lhotari (via GitHub)" <gi...@apache.org>.
lhotari merged PR #24100:
URL: https://github.com/apache/pulsar/pull/24100
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [fix][client] Consumer leak when thread.Interrupted is called before subscribed [pulsar]
Posted by "lhotari (via GitHub)" <gi...@apache.org>.
lhotari commented on PR #24100:
URL: https://github.com/apache/pulsar/pull/24100#issuecomment-2740207815
> * Cannot close the consumer since user will get InterruptedException and can't get consumer object
@shibd Good work! It would be useful to have a failing test case that reproduces this problem included in this PR. Could you please add it too? It seems that it would be a slight variation of the test case that you have added. The "can't get consumer object" part isn't currently tested so that after fixing the issue the "consumer object can be get" (the test would clarify what this actually means).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [fix][client] Consumer leak when thread.Interrupted is called before subscribed [pulsar]
Posted by "lhotari (via GitHub)" <gi...@apache.org>.
lhotari commented on PR #24100:
URL: https://github.com/apache/pulsar/pull/24100#issuecomment-2740295603
> You can run `testInterruptedWhenCreateConsumer ` to reproduce it.
@shibd That's not reproducing this flow what you described in the PR description
> For example:
>
> * Thread A - call subscribe() method
> * Thread A - blocking on waiting for the response of the consumer creation
> * The user framework interrupted `Thread A` for aborting a job.
> * Cannot close the consumer since user will get InterruptedException and can't get consumer object
The step "Cannot close the consumer since user will get InterruptedException and can't get consumer object" is missing. Adding a separate test case that reproduces the user's flow would be useful as I have explained before. Just simply put it in code in a test case which clarifies what "Cannot close the consumer since user will get InterruptedException and can't get consumer object" means. That's the most simplest way without wasting time in writing comments. :)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [fix][client] Consumer leak when thread.Interrupted is called before subscribed [pulsar]
Posted by "lhotari (via GitHub)" <gi...@apache.org>.
lhotari commented on PR #24100:
URL: https://github.com/apache/pulsar/pull/24100#issuecomment-2741072800
> I don't want to waste your time. I believe I've already added tests to cover it.
That's true, that there's a test, but the test is not from the viewpoint of the user of our software. The flow described in the description isn't performed in the test case. That would be an optimal "failing test case". In many software teams, this is a well known concept. However, in the Pulsar code base, most of our tests are not following usual best practices and don't appear to be good examples of how to write good tests.
> You'll see that I've covered in the tests that the consumer will be removed(closed) after this PR fix.
That's not "Cannot close the consumer since user will get InterruptedException and can't get consumer object", what was in the description. I agree that the internal behavior change is covered in the test, but not the original bug that appeared.
> In this scenario, not getting the consumer object is expected. Why would we add a test case for it? A method throws an exception, and you want to cover that it returns null?
Simply test the scenario "Cannot close the consumer since user will get InterruptedException and can't get consumer object" and assume that it results in whatever is the expected result. It's not more harder than that.
> If you can review the code directly and let me know where improvements are needed, I'd be happy to optimize it.
As long as the mentioned scenario is handled in a separate test case, I'm fine with this PR. I know that it's unusual to knit pick on such details in our PRs, but unless attention is paid to details we won't be able to learn from each other and improve as a whole.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [fix][client] Consumer leak when thread.Interrupted is called before subscribed [pulsar]
Posted by "shibd (via GitHub)" <gi...@apache.org>.
shibd commented on PR #24100:
URL: https://github.com/apache/pulsar/pull/24100#issuecomment-2740316682
I don't want to waste your time. I believe I've already added tests to cover it.
You'll see that I've covered in the tests that the consumer will be removed(closed) after this PR fix.
That's the issue I'm addressing.
In this scenario, not getting the consumer object is expected. Why would we add a test case for it? A method throws an exception, and you want to cover that it returns null?
If you can review the code directly and let me know where improvements are needed, I'd be happy to optimize it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [fix][client] Consumer leak when thread.Interrupted is called before subscribed [pulsar]
Posted by "shibd (via GitHub)" <gi...@apache.org>.
shibd commented on PR #24100:
URL: https://github.com/apache/pulsar/pull/24100#issuecomment-2742490241
hi, @nodece For operation type of api, such as: `ack()`, `nack()`. In my understanding, not handling interruptions is acceptable because, even during an interruption, whether the API operation succeeds or fails, it is acceptable to the user.
However, we cannot consider the consumer leak described in this PR as expected behavior; we should fix it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org