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

<thread>: promise::set_value_at_thread_exit sets the value too early #3614

Open
fsb4000 opened this issue Apr 1, 2023 · 4 comments
Open
Labels
bug Something isn't working vNext Breaks binary compatibility

Comments

@fsb4000
Copy link
Contributor

fsb4000 commented Apr 1, 2023

Describe the bug
from https://en.cppreference.com/w/cpp/thread/promise/set_value_at_thread_exit

Stores the value into the shared state without making the state ready immediately. The state is made ready when the current thread exits, after all variables with thread-local storage duration have been destroyed.

but this is not true for our implementation.

This is probably a known issue because I found a comment in the implementation // TRANSITION, ABI but I didn't find a github issue about it so I decided to create it.

Command-line test case

C:\Temp>type main.cpp
#include<iostream>
#include<future>
using namespace std;

struct foo {
	~foo() { cout << "~foo() "; }
};

int main() {
	promise<void> prom;
	future<void> f = prom.get_future();
	thread t([&prom] {
		thread_local foo foo;
		prom.set_value_at_thread_exit();
	});
	f.get();
	cout << "gotten ";
	t.join();
}

C:\Temp>cl /EHsc /W4 /WX .\main.cpp
Microsoft (R) C/C++ Optimizing Compiler Version 19.36.32502 for x64
Copyright (C) Microsoft Corporation.  All rights reserved.

main.cpp
Microsoft (R) Incremental Linker Version 14.36.32502.0
Copyright (C) Microsoft Corporation.  All rights reserved.

/out:main.exe
main.obj

C:\Temp>main.exe
gotten ~foo()

Expected behavior
The output should be “~foo() gotten”

STL version

    Microsoft Visual Studio Community 2022 Preview
    Version 17.6.0 Preview 2.0

Additional context
DevCom-10322768 and internal VSO-1779407 / AB#1779407

I played with the example a bit and I think this line is wrong:

_Cnd_do_broadcast_at_thread_exit(); // TRANSITION, ABI

I added more details to Devcom-10322768 as my comment

If we could use thread_local in STL we could add a thread_local struct and call _Cnd_do_broadcast_at_thread_exit in its destructor...

vNext note: Resolving this issue will require breaking binary compatibility. We won't be able to accept pull requests for this issue until the vNext branch is available. See #169 for more information.

@StephanTLavavej StephanTLavavej added bug Something isn't working vNext Breaks binary compatibility labels Apr 1, 2023
@StephanTLavavej
Copy link
Member

Note that I don't expect the reasons why we avoid thread-local storage in the STL to change for vNext.

@AlexGuteniev
Copy link
Contributor

Note that I don't expect the reasons why we avoid thread-local storage in the STL to change for vNext.

Could it be Windows XP thing or CLR thing?

@StephanTLavavej
Copy link
Member

Unrelated to XP or CLR. I explained the rationale in #673 (comment).

@frederick-vs-ja
Copy link
Contributor

If we could use thread_local in STL we could add a thread_local struct and call _Cnd_do_broadcast_at_thread_exit in its destructor...

I'm afraid that we cannot reliably fix this in STL even if thread_local usage is permitted. I guess some changes might be necessary in VCRuntime's __dyn_tls_dtor.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working vNext Breaks binary compatibility
Projects
None yet
Development

No branches or pull requests

4 participants