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

[redis_proxy] Assert proper transaction initialization #37827

Closed
wants to merge 1 commit into from

Conversation

dbarbosapn
Copy link
Contributor

@dbarbosapn dbarbosapn commented Dec 27, 2024

Fixes #37825

Making sure we know early that transaction initialization was not done correctly.

Tested manually, was unable to reproduce issue again. I'm guessing the assertion caused compiler to not do bad optimization there 🤷

Copy link
Member

@mattklein123 mattklein123 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry this doesn't make sense. We need to better understand the problem. Are you sure this path is properly covered in test and by ASAN/TSAN?

Also use the Envoy ASSERT macro which is not compiled on release builds, which if this is an optimization issue (very doubtful) won't help.

/wait

@dbarbosapn
Copy link
Contributor Author

Sorry this doesn't make sense. We need to better understand the problem. Are you sure this path is properly covered in test and by ASAN/TSAN?

Also use the Envoy ASSERT macro which is not compiled on release builds, which if this is an optimization issue (very doubtful) won't help.

/wait

Oh yeah it doesn't make any sense 😂 Just created PR to have the CI build to see if it wasn't just local the issue (if the assertion would fire).

I'll keep investigating. Maybe it was something transient on that specific build.

Will try tomorrow latest main, to see if the issue is there or not. If not, I'll abandon this PR.

@dbarbosapn
Copy link
Contributor Author

Closing as not able to reproduce with latest binaries. Will reopen investigation if we notice this again 😃

@dbarbosapn dbarbosapn closed this Dec 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[redis_proxy] New UNWATCH command causes unexpected behavior on release build only
2 participants