You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by GitBox <gi...@apache.org> on 2022/08/25 03:37:20 UTC
[GitHub] [pulsar-client-go] Gleiphir2769 opened a new pull request, #835: [issue 833] Fix for availablePermits leak
Gleiphir2769 opened a new pull request, #835:
URL: https://github.com/apache/pulsar-client-go/pull/835
Fixes #833
### Motivation
The availablePermits may leak when message is discarded. Please refer to #833 for details.
### Modifications
- Wraps a struct availablePermits to atomic increase and reset.
- Add the permits increasing in `discardCorruptedMessage`.
### Verifying this change
- [x] Make sure that the change passes the CI checks.
This change added tests and can be verified as follows: `pulsar/consumer_test.go:TestAvailablePermitsLeak()`
--
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
[GitHub] [pulsar-client-go] Gleiphir2769 commented on pull request #835: [Issue 833] Fix the availablePermits leak that could cause consumer stuck.
Posted by GitBox <gi...@apache.org>.
Gleiphir2769 commented on PR #835:
URL: https://github.com/apache/pulsar-client-go/pull/835#issuecomment-1237020671
The modify for dispatcher has done. CC @zzzming
By the way, what is the difference between `availablePermits` and `requestedPermits`?
https://github.com/apache/pulsar-client-go/blob/68e43175f2c4e8eb3bf181a02b03c5382a09f1aa/pulsar/consumer_partition.go#L933-L944
I think the code here is redundant, `availablePermits` always equal with the `requestedPermits`.
--
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
[GitHub] [pulsar-client-go] Gleiphir2769 commented on pull request #835: [Issue 833] Fix the availablePermits leak that could cause consumer stuck.
Posted by GitBox <gi...@apache.org>.
Gleiphir2769 commented on PR #835:
URL: https://github.com/apache/pulsar-client-go/pull/835#issuecomment-1236293799
> Can we let dipatcher() to manage the availablePermits since most cases already covered by various signals in dispatcher()'s select cases. The benefits are 1) centralize one function to manage availablePermits 2) to avoid using Atomic or any synchronizing because avialablePermits can only updated one at a time. To achieve that, can we have failure cases from MessageReceived() to send a signal to a new channel that dispatcher() listens to that will call your new function increasePermitsAndRequestMoreIfNeed()
Hi @zzzming. I agree with you that handle the availablePermits in dispatcher().
>
> There might be other cases leaking availablePermits. So in the future, those cases can send a sig to this new increasePermits channel.
>
You are right. Chunking also need to increase availablePermits when all the chunks haven't all arrived yet. So this is also a case where need to request more `availablePermits` after return early from `MessageReceived()`.
--
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
[GitHub] [pulsar-client-go] zzzming commented on pull request #835: [Issue 833] Fix the availablePermits leak that could cause consumer stuck.
Posted by GitBox <gi...@apache.org>.
zzzming commented on PR #835:
URL: https://github.com/apache/pulsar-client-go/pull/835#issuecomment-1273683680
@Gleiphir2769 `availablePermits` is consumer counter to keep track how many messages the broker can send. Upon the initial connection of a consumer, the consumer sets the initial permits to the queue size and sends to the broker; because of that, the available permits reduces to 0. That is in the dispatcher() function.
For every messages it receives, the available permit is increased by one. This allows the consumer to request more messages once the number of permits over the threshold. This is where you have changed the code in this PR.
`requestPermits` is the number of messages the consumer communicates with the broker that it allows to send.
These two are very close terminologies. Ultimately, we need one counter to keep track the number of availablePermits. The requestPermits is just an intermediary variable hold the availablePermits value and send to the broker.
--
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
[GitHub] [pulsar-client-go] wolfstudy merged pull request #835: [Issue 833] Fix the availablePermits leak that could cause consumer stuck.
Posted by GitBox <gi...@apache.org>.
wolfstudy merged PR #835:
URL: https://github.com/apache/pulsar-client-go/pull/835
--
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