Skip to content

Fix an unbalanced release of the producer's pending semaphore #392

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.

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

Merged
merged 1 commit into from
Feb 5, 2024

Conversation

erobot
Copy link
Contributor

@erobot erobot commented Feb 1, 2024

Motivation

Current code releases the producer's pending semaphore twice when batch is off and message is too big. The unbalanced release overflows the semaphore, and subsequent sends will fail with ProducerQueueIsFull.

Modifications

Remove the redundant semaphore release as the necessary release will be done in handleFailedResult.

Verifying this change

  • Make sure that the change passes the CI checks.

This change modified a existing test and can be verified as follows:

  • ProducerTest.testMaxMessageSize

Documentation

  • doc-required
    (Your PR needs to update docs and you will update later)

  • doc-not-needed
    Bug fix.

  • doc
    (Your PR contains doc changes)

  • doc-complete
    (Docs have been already added)

@erobot
Copy link
Contributor Author

erobot commented Feb 1, 2024

Some tests failed in my forked repo but can succeed in my local environment. I don't know why but seems unrelated to this PR.

Failed CI job: https://github.com/erobot/pulsar-client-cpp/actions/runs/7740402401/job/21107366697

FAILED TESTS (8/538):
78959 ms: ./pulsar-tests AuthPluginTest.testTlsDetectPulsarSslWithInvalidBroker (try #1)
78822 ms: ./pulsar-tests AuthPluginTest.testTlsDetectHttpsWithInvalidBroker (try #1)
77618 ms: ./pulsar-tests AuthPluginTest.testTlsDetectPulsarSslWithInvalidBroker (try #2)
77661 ms: ./pulsar-tests AuthPluginTest.testTlsDetectHttpsWithInvalidBroker (try #2)
76701 ms: ./pulsar-tests AuthPluginTest.testTlsDetectPulsarSslWithInvalidBroker (try #3)
76744 ms: ./pulsar-tests AuthPluginTest.testTlsDetectHttpsWithInvalidBroker (try #3)
81038 ms: ./pulsar-tests AuthPluginTest.testTlsDetectPulsarSslWithInvalidBroker (try #4)
81082 ms: ./pulsar-tests AuthPluginTest.testTlsDetectHttpsWithInvalidBroker (try #4)

@BewareMyPower BewareMyPower added the bug Something isn't working label Feb 1, 2024
@BewareMyPower BewareMyPower added this to the 3.5.0 milestone Feb 1, 2024
@BewareMyPower
Copy link
Contributor

Some tests failed in my forked repo but can succeed in my local environment. I don't know why but seems unrelated to this PR.

Not sure if it's caused by a breaking change in Pulsar 3.1.2. The apachepulsar/pulsar:latest image was upgraded from 3.1.1 to 3.1.2 today. You can open an empty PR in your fork to see if any test failed.

@erobot
Copy link
Contributor Author

erobot commented Feb 2, 2024

Some tests failed in my forked repo but can succeed in my local environment. I don't know why but seems unrelated to this PR.

Not sure if it's caused by a breaking change in Pulsar 3.1.2. The apachepulsar/pulsar:latest image was upgraded from 3.1.1 to 3.1.2 today. You can open an empty PR in your fork to see if any test failed.

I opened an empty PR and the same tests failed.
Empty PR: erobot#13

@BewareMyPower
Copy link
Contributor

I see. I will take a look at these failed tests today.

@BewareMyPower
Copy link
Contributor

I've confirmed it's a breaking change from Pulsar 3.1.2. After I switched from Pulsar 3.1.2 to Pulsar 3.1.1, the tests passed.

@BewareMyPower
Copy link
Contributor

Could you rebase to master so that the CI won't be affected?

@erobot erobot force-pushed the fix-producer-semaphore branch from 196663f to 5b587d1 Compare February 3, 2024 01:57
@erobot
Copy link
Contributor Author

erobot commented Feb 3, 2024

Could you rebase to master so that the CI won't be affected?

Rebased.

@BewareMyPower BewareMyPower merged commit 77e0c22 into apache:main Feb 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants