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.

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

[C++] Fix dangling reference bug in getRandomName #8596

Merged
merged 2 commits into from
Nov 23, 2020

Conversation

oversearch
Copy link
Contributor

@oversearch oversearch commented Nov 17, 2020

Motivation

The current getRandomName function contains a simple dangling reference bug: when it attempts to return a const char* string value from the temporary string returned by the std::stringstream::str function. This sometimes "works" anyway in release mode depending on the platform it's running on, but in debug mode on all the versions of GCC and CLANG I've tried it segfaults. This makes debugging the c++ client very annoying.

Modifications

I found the existing method for generating random strings rather obscure, so I both fixed the bug and simplified the code. I made use of boost::random to provide a much more straightforward implementation. I use this pattern often for random names in my company's production projects, and it works well.

Verifying this change

  • Make sure that the change passes the CI checks.

This code is called every time the client subscribes to a topic, so it gets plenty of coverage throughout the tests. I also manually verified that it's working correctly by printing the random names while I was testing. I've already included this fix in a local build of Pulsar I'm using for a new product at FactSet, and it's working well there.

The CI checks are failing with some error about a website deployment that seems unrelated to the C++ client. Is this a problem? Please let me know if I need to do anything special to get those checks to pass. Thanks!

…ce bug: when it attempts to return a "const char*" string value from the temporary string returned by the stringstream "str" function. This sometimes "works" anyway in release mode, but in debug mode on all the versions of GCC and CLANG I've tried it segfaults. This makes debugging the c++ client very annoying.

I also found the method for generating random strings rather obscure. So I both fixed the bug and simplified the code, making use of boost::random to provide a much more straightforward implementation.
@BewareMyPower
Copy link
Contributor

The CI uses clang-format 5.0 to check code style so you should reformat your code before pushing a commit. And C++11 already has std::random, I think using C++ standard library is preferred to Boost if they have the same API.

@oversearch
Copy link
Contributor Author

Thank you for the feedback! I forgot about the STL getting boost::random. I've switched the implementation and applied clang-format (for some reason CMake has trouble finding the clang-format binary on my machine...).

Hopefully all the required checks will succeed this time.

@BewareMyPower
Copy link
Contributor

/pulsarbot run-failure-checks

@merlimat merlimat added this to the 2.8.0 milestone Nov 23, 2020
@merlimat merlimat added the type/bug The PR fixed a bug or issue reported a bug label Nov 23, 2020
@merlimat merlimat merged commit 83f0345 into apache:master Nov 23, 2020
@oversearch oversearch deleted the boveresch/fix/random_name branch November 24, 2020 18:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

None yet

3 participants