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

Disable TLS for MINGW due to issues with the order of freeing up memory. #57

Merged
merged 1 commit into from
Sep 23, 2019

Conversation

darinpp
Copy link

@darinpp darinpp commented Sep 23, 2019

There seems to be an issue with the way MINGW frees up TLS memory. More specifically - the TLS is freed up before the destructors on the objects residing in TLS are called. In our case - we were observing a destructor accessing a block of memory that was already freed.

Here is a report from end uses that seems to be related https://sourceforge.net/p/mingw-w64/bugs/727/

Fixes cockroachdb/cockroach#40936
Fixes cockroachdb/cockroach#40925


This change is Reviewable

@darinpp darinpp requested review from ajkr and petermattis September 23, 2019 16:52
Copy link

@petermattis petermattis left a comment

Choose a reason for hiding this comment

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

:lgtm: though I'm curious why we're trying to run destructors at process exit vs a more abrupt termination.

Reviewable status: 0 of 1 files reviewed, all discussions resolved (waiting on @ajkr and @petermattis)

@darinpp
Copy link
Author

darinpp commented Sep 23, 2019

I doubt that abrupt exit is the best things to do. I could imagine trying to log some information at termination time. One place where I saw the issue is here:

ClearPerLevelPerfContext();

:lgtm: though I'm curious why we're trying to run destructors at process exit vs a more abrupt termination.

Reviewable status: 0 of 1 files reviewed, all discussions resolved (waiting on @ajkr and @petermattis)

@petermattis
Copy link

I doubt that abrupt exit is the best things to do.

It's pretty common technique to avoid trying to cleanly tear down some complicated setup. On the other hand, it might paint us into a corner with regards to doing real CI tests on Windows in the future.

@darinpp darinpp merged commit 8978e86 into cockroachdb:crl-release-6.2.1 Sep 23, 2019
@darinpp darinpp deleted the crl-release-6.2.1 branch September 23, 2019 20:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants