[PATCH] D94622: [clang-tidy] add concurrency-async-no-new-threads

[Vasily Kulikov via Phabricator via cfe-commits]

segoon added inline comments.


================
Comment at: clang-tools-extra/clang-tidy/concurrency/AsyncNoNewThreadsCheck.cpp:22
+    /* C++ std */
+    "::std::async", //
+
----------------
aaron.ballman wrote:
> segoon wrote:
> > aaron.ballman wrote:
> > > The trailing comment markers don't really add much.
> > it's a hack for clang-format, otherwise it contatenates the lines, creating unmaintainable mess of strings. "One line - one name" is much more suitable.
> Ah, I didn't know that'd change the behavior of clang-format, that's neat!
> 
> FWIW, we usually don't do a whole lot of markup to avoid clang-format issues (such as the clang-format: on/off markers). Instead, we usually just ignore the LINT warnings in code review and check the code in as-is. This helps reduce clutter in the code base. In this case, the comments aren't adding a ton of clutter so maybe they're fine. But they definitely look odd as a reader of the code, which is a bit distracting.
IMO having an ability to run clang-format for sources is very handy. It makes me sad if running clang-format against the source forces me to set some markers or reverting a part of clang-format changes in the source parts I have never touched. An ability to simply rerun checks/linters is very powerful.


================
Comment at: clang-tools-extra/clang-tidy/concurrency/AsyncNoNewThreadsCheck.cpp:33
+
+    /* Linux */
+    "::fork",     //
----------------
aaron.ballman wrote:
> segoon wrote:
> > aaron.ballman wrote:
> > > If we're going to add these, we should probably also add ones for Win32 and Mac OS as well, like `CreateThread`, `CreateRemoteThread`, `_beginthread`, `_beginthreadex`, etc.
> > I don't mind, but I'm not an expert in WinAPI or Windows programming. So, this part should be by someone with expertise of Windows in separate patches.
> I don't think it needs to be done in separate patches. I can give you the full list of things I care about.
> ```
> CreateThread (https://docs.microsoft.com/en-us/windows/win32/api/processthreadsapi/nf-processthreadsapi-createthread)
> CreateRemoteThread (https://docs.microsoft.com/en-us/windows/win32/api/processthreadsapi/nf-processthreadsapi-createremotethread)
> CreateRemoteThreadEx (https://docs.microsoft.com/en-us/windows/win32/api/processthreadsapi/nf-processthreadsapi-createremotethreadex)
> _beginthread
> _beginthreadex (https://docs.microsoft.com/en-us/cpp/c-runtime-library/reference/beginthread-beginthreadex?view=msvc-160)
> ```
Oh, great, thank you! Added.


================
Comment at: clang-tools-extra/docs/clang-tidy/checks/concurrency-async-no-new-threads.rst:8-10
+functions and types. E.g. if the code uses C++ coroutines, it is expected
+that only new coroutines or coroutine-based primitives are created
+instead of heavy system threads.
----------------
aaron.ballman wrote:
> segoon wrote:
> > aaron.ballman wrote:
> > > FWIW, this suggests to me that what you really want is a way for APIs to opt into this behavior. There's no reason why you wouldn't have a complex system that has both threads and coroutines in it, but it does stand to reason that you may want to say "this function, and everything called within this function, should not create any system threads" in some situations.
> > > 
> > > The note below helps call out the expectations from the check, but it requires the developer to restructure the way they write code pretty drastically in order to make the checking behavior more reasonable, which does not seem ideal.
> > I think it is a complex problem, so it should be separated into smaller tasks.
> > 
> > Step one - checks with hardcoded functions/types with user-guided enabling on a per-file basis. A semi-automated check.
> > 
> > Step two - try to solve other parts of the puzzle. Maybe try to add [clang:coroutine_safe] tag and teach clang static analyzer to deduce coroutine safety property and use it for enabling/disabling the cheks. Maybe reuse other (not yet implemented) heuristics from static analyzer (or other tools) to identify coroutine functions and check only these functions. I'm not an expert in static analyzer, so other LLVM developers might find a clever heuristics when to enable/disable these checks or maybe how to deduce blacklisted/whitelisted functions/types lists (e.g. for concurrency-async-{fs,blocking}).
> > 
> > Indeed, the current approach has its own limitations. But it may be a first step in the right direction.
> My fear with the current approach is that I don't think projects usually split their code in the way this check requires, and so the check will not be very useful in practice because it will be too chatty. Have you tried running this check over some large projects that use coroutines to see what the diagnostic results look like?
I haven't conducted a survey, but I can say for Yandex.Taxi where the patch has originated. The codebase is ~350KLOC. The main parts are:
1) a coroutine framework that uses blocking functions / threads creation /etc. inside - concurrency-async-* disabled
2) coroutine libraries consisting of coroutine-only code - concurrency-async-* enabled
3) services with coroutine-only code - concurrency-async-* enabled

All 3 parts are clearly separated from each other from the filesystem prospective. Actually I think there are plenty of other invariants that must be met for coroutine code, so separate coroutine/non-coroutine namespaces (and consequently, filesystem separation) is a must. E.g. there are filesystem::RewriteFileContents() and filesystem::blocking::RewriteFileContents() - trivial to realize whether you may call it or not. I admit that other projects may have quite different approach to namespaces/modules, though.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D94622/new/

https://reviews.llvm.org/D94622