Add coroutine-hostile-raii lint for UnwindDetector, and add fixes - #7153
Add coroutine-hostile-raii lint for UnwindDetector, and add fixes#7153jtolio wants to merge 1 commit into
Conversation
|
The generated output of |
a0fc3a8 to
ce193e4
Compare
|
@jtolio Bonk workflow was cancelled. View workflow run · To retry, trigger Bonk again. |
The fixes in container-client fix not only the lint but also what appears to be the code's intent. The lints are fixed in standard.c++ but the behavior is unchanged and may be buggy. I'm not sure it is intended that the KJ_CATCH branch does not run when writeFailed == true. Added TODOs for a later followup.
ce193e4 to
c03731f
Compare
| if (result.chunks.size() > 0) { | ||
| KJ_ON_SCOPE_FAILURE(writeFailed = true); | ||
| auto invocation = KJ_CO_MAGIC kj::CURRENT_INVOCATION; | ||
| // TODO(soon) - should this also handle invocation.isCanceling()? |
There was a problem hiding this comment.
Probably not, but @jasnell could perhaps confirm: If someone drops this pumpToImpl() coroutine's promise, should we set writeFailed = true?
I think the KJ_ON_SCOPE_FAILURE was probably just meant to be equivalent to a .catch_(), and not consider cancellation.
| }), | ||
| ) | ||
|
|
||
| sh_test( |
There was a problem hiding this comment.
I don't think we need to add a test for this – as long as we're setting RAIITypesList properly this isn't checking workerd behavior but rather if the misc-coroutine-hostile-raii clang-tidy check works correctly, which should already be tested within LLVM.
3 participants
This change adds a lint to prevent usage of UnwindDetector across coroutine pauses. See discussion on capnproto/capnproto#2789 why UnwindDetector is not safe on coroutines.
The fixes in container-client fix not only the lint but also what appears to be the code's intent.
The lints are fixed in standard.c++ but the behavior is unchanged and may be buggy. I'm not sure it is intended that the KJ_CATCH branch does not run when writeFailed == true.
Added TODOs for a later followup.