Skip to content

[fix][client] Fix internal receive used wrong timeout type - #15014

Merged
RobertIndie merged 1 commit into
apache:masterfrom
Demogorgon314:fix/internal-receive-timeout-Integer-overflow
Apr 4, 2022
Merged

[fix][client] Fix internal receive used wrong timeout type #15014
RobertIndie merged 1 commit into
apache:masterfrom
Demogorgon314:fix/internal-receive-timeout-Integer-overflow

Conversation

@Demogorgon314

Copy link
Copy Markdown
Member

Fixes #15001

Motivation

Currently, when we called redeliverUnacknowledgedMessages and consumer use receive with timeout unit is second, the timeout might not work.

When we use the second as receive timeout unit like consumer.receive(3, TimeUnit.SECONDS);, line 473 will pass a negative timeout to the next internalReceive calls, because we are subtracting nanoseconds from seconds.

protected Message<T> internalReceive(int timeout, TimeUnit unit) throws PulsarClientException {
Message<T> message;
long callTime = System.nanoTime();
try {
message = incomingMessages.poll(timeout, unit);
if (message == null) {
return null;
}
messageProcessed(message);
if (!isValidConsumerEpoch(message)) {
long executionTime = System.nanoTime() - callTime;
if (executionTime >= unit.toNanos(timeout)) {
return null;
} else {
return internalReceive((int) (timeout - executionTime), TimeUnit.NANOSECONDS);
}
}
return beforeConsume(message);

Modifications

  • Use the same type to subtract.
  • Use long type as timeout argument.

Documentation

Check the box below or label this PR directly (if you have committer privilege).

Need to update docs?

  • doc-required

    (If you need help on updating docs, create a doc issue)

  • no-need-doc

This is a bug fix, no need for docs.

  • doc

    (If this PR contains doc changes)

@github-actions github-actions Bot added the doc-not-needed Your PR changes do not impact docs label Apr 3, 2022

@Jason918 Jason918 left a comment

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.

Good catch.

@RobertIndie
RobertIndie merged commit bdc3024 into apache:master Apr 4, 2022
@Demogorgon314
Demogorgon314 deleted the fix/internal-receive-timeout-Integer-overflow branch April 4, 2022 04:05
@codelipenghui codelipenghui added this to the 2.11.0 milestone Apr 4, 2022
codelipenghui pushed a commit that referenced this pull request Apr 19, 2022
### Motivation

Currently, when we called `redeliverUnacknowledgedMessages` and consumer use receive with timeout unit is second, the timeout might not work.

When we use the second as receive timeout unit like `consumer.receive(3, TimeUnit.SECONDS);`, line 473 will pass a negative timeout to the next `internalReceive` calls, because we are subtracting nanoseconds from seconds.

https://github.com/apache/pulsar/blob/f3b87b65c6946eb197c1eece22cff8ff04e16fcb/pulsar-client/src/main/java/org/apache/pulsar/client/impl/ConsumerImpl.java#L459-L476

### Modifications

* Use the same type to subtract.
* Use long type as timeout argument.

(cherry picked from commit bdc3024)
Nicklee007 pushed a commit to Nicklee007/pulsar that referenced this pull request Apr 20, 2022
)

### Motivation

Currently, when we called `redeliverUnacknowledgedMessages` and consumer use receive with timeout unit is second, the timeout might not work.

When we use the second as receive timeout unit like `consumer.receive(3, TimeUnit.SECONDS);`, line 473 will pass a negative timeout to the next `internalReceive` calls, because we are subtracting nanoseconds from seconds.

https://github.com/apache/pulsar/blob/f3b87b65c6946eb197c1eece22cff8ff04e16fcb/pulsar-client/src/main/java/org/apache/pulsar/client/impl/ConsumerImpl.java#L459-L476


### Modifications

* Use the same type to subtract.
* Use long type as timeout argument.
@mattisonchao

mattisonchao commented May 24, 2022

Copy link
Copy Markdown
Member

After discussing with @Demogorgon314, We don't need to cherry-pick this PR to branch-2.9.
Because the issue involves by #10478

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/client cherry-picked/branch-2.10 doc-not-needed Your PR changes do not impact docs release/2.10.1 type/bug The PR fixed a bug or issue reported a bug

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Flaky-test: SimpleProducerConsumerTest. testRedeliveryFailOverConsumer

6 participants