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

GH-44846: [CI][C++] Revert change for maybe uninitialized variable to fix thread sanitizer failure #44848

Closed
wants to merge 1 commit into from

Conversation

raulcd
Copy link
Member

@raulcd raulcd commented Nov 25, 2024

Rationale for this change

The thread sanitizer build was failing since the original was merged: 2e1ddf5

What changes are included in this PR?

Revert change

Are these changes tested?

Via CI

Are there any user-facing changes?

No

@raulcd
Copy link
Member Author

raulcd commented Nov 25, 2024

@github-actions crossbow submit test-ubuntu-24.04-cpp-thread-sanitizer

Copy link

⚠️ GitHub issue #44846 has been automatically assigned in GitHub to PR creator.

@raulcd
Copy link
Member Author

raulcd commented Nov 25, 2024

I can't reproduce locally, I am just trying to understand if a previous commit is the culprit of the issue.

Copy link

Revision: c58ba59

Submitted crossbow builds: ursacomputing/crossbow @ actions-6b9fcced11

Task Status
test-ubuntu-24.04-cpp-thread-sanitizer GitHub Actions

@raulcd raulcd changed the title WIP: GH-44846: [CI][C++] Investigate GH-44846 WIP: GH-44846: [CI][C++] Revert change for maybe uninitializer variable to fix thread sanitizer failure Nov 25, 2024
@raulcd raulcd changed the title WIP: GH-44846: [CI][C++] Revert change for maybe uninitializer variable to fix thread sanitizer failure GH-44846: [CI][C++] Revert change for maybe uninitializer variable to fix thread sanitizer failure Nov 25, 2024
@raulcd raulcd marked this pull request as ready for review November 25, 2024 13:55
@raulcd
Copy link
Member Author

raulcd commented Nov 25, 2024

@pitrou @kou this seems to be the culprit for the thread sanitizer failures. Should I revert the change?

@raulcd raulcd changed the title GH-44846: [CI][C++] Revert change for maybe uninitializer variable to fix thread sanitizer failure GH-44846: [CI][C++] Revert change for maybe uninitialized variable to fix thread sanitizer failure Nov 25, 2024
@pitrou
Copy link
Member

pitrou commented Nov 25, 2024

I don't know, I don't understand how those TSan failures could be related to the change being reverted.

@pitrou
Copy link
Member

pitrou commented Nov 25, 2024

Also, I don't know why CI was mostly skipped on this PR, did you put "WIP" in the PR title at some point?

@raulcd
Copy link
Member Author

raulcd commented Nov 25, 2024

yes, I was initially trying to understand if this was the issue or not, that's why I used WIP. I've just rebased with a new commit message to trigger CI again.

@pitrou
Copy link
Member

pitrou commented Nov 25, 2024

Ok, I think the original commit just surfaces a lingering bug in ConcurrentQueue. UnsyncFront is simply not safe, as it calls deque::front() which can be implemented in terms of deque::begin(), but a concurrent call to deque::push_back() is specificed as invalidating all iterators.

@raulcd
Copy link
Member Author

raulcd commented Nov 25, 2024

Thanks @pitrou then let me close this PR as the change is good and it surfaced the bug. I will add your comment to the thread sanitizer issue

@raulcd raulcd closed this Nov 25, 2024
@raulcd
Copy link
Member Author

raulcd commented Nov 25, 2024

ok, I just saw you are working on the fix already, no need to add comment then. Thanks!

@raulcd raulcd deleted the GH-44846 branch November 25, 2024 15:58
@raulcd raulcd restored the GH-44846 branch November 25, 2024 15:58
@raulcd raulcd deleted the GH-44846 branch November 25, 2024 15:59
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.

2 participants