Skip to content

Fix TSan errors (correctly ignore _exit interception)#43009

Merged
tavplubix merged 2 commits intoClickHouse:masterfrom
azat:fix-tsan
Nov 7, 2022
Merged

Fix TSan errors (correctly ignore _exit interception)#43009
tavplubix merged 2 commits intoClickHouse:masterfrom
azat:fix-tsan

Conversation

@azat
Copy link
Copy Markdown
Member

@azat azat commented Nov 7, 2022

Changelog category (leave one):

  • Not for changelog (changelog entry is not required)

Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):

Fix TSan errors (correctly ignore _exit interception)

Because safeExit() does not includes header with defines, it does not know about THREAD_SANITIZER.

And it also fixes Azure blob storage, actually everything is fine with the sdk itself, the problem is only in TSan that intercepts _exit() and report leak, even thoug that tread will be joined later.

Refs: #23056 (#23616)
Fixes: #38474
Closes: #42640
Fixes: #42638
Fixes: #34988
Cc: @alexey-milovidov, @tavplubix

Copy link
Copy Markdown
Member

@tavplubix tavplubix left a comment

Choose a reason for hiding this comment

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

LOL, nice catch, thank you! Could you please add a comment about importance of this include?

@robot-ch-test-poll2 robot-ch-test-poll2 added the pr-not-for-changelog This PR should not be mentioned in the changelog label Nov 7, 2022
Because safeExit() does not includes header with defines, it does not
know about THREAD_SANITIZER.

And it also fixes Azure blob storage, actually everything is fine with
the sdk itself, the problem is only in TSan that intercepts _exit() and
report leak, even thoug that tread will be joined later.

Refs: ClickHouse#23056 (ClickHouse#23616)
Fixes: ClickHouse#38474
Closes: ClickHouse#42640
Fixes: ClickHouse#42638
Fixes: ClickHouse#34988
Cc: @alexey-milovidov, @tavplubix

Signed-off-by: Azat Khuzhin <[email protected]>
@tavplubix tavplubix self-assigned this Nov 7, 2022
@tavplubix tavplubix mentioned this pull request Nov 7, 2022
@tavplubix
Copy link
Copy Markdown
Member

Ha-ha, now build fails because of

Nov 07 16:53:09 /build/base/base/safeExit.cpp:14:5: error: use of undeclared identifier 'abort'
Nov 07 16:53:09     UNREACHABLE();
Nov 07 16:53:09     ^
Nov 07 16:53:09 /build/base/base/defines.h:131:31: note: expanded from macro 'UNREACHABLE'
Nov 07 16:53:09         #define UNREACHABLE() abort()
Nov 07 16:53:09                               ^
Nov 07 16:53:09 1 error generated.

@tavplubix
Copy link
Copy Markdown
Member

Builds are ok

@tavplubix tavplubix merged commit fc77d53 into ClickHouse:master Nov 7, 2022
@azat
Copy link
Copy Markdown
Member Author

azat commented Nov 10, 2022

By the way, maybe it worth to backport for CI over release branches?

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

Labels

pr-backports-created Backport PRs are successfully created, it won't be processed by CI script anymore pr-must-backport Pull request should be backported intentionally. Use this label with great care! pr-not-for-changelog This PR should not be mentioned in the changelog

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Temporary remove Azure Blob Storage support, because a test has failed. Thread leak (TCPHandler)

4 participants