Reimplement signal handler to avoid async-unsafe functions #97
Conversation
| #include <memory> | ||
|
|
||
| namespace std { | ||
| class thread; |
There was a problem hiding this comment.
Don't need a forward dec'l if you already include
There was a problem hiding this comment.
And please use ROOT coding standards.
There was a problem hiding this comment.
(by the way, is there a good online ref for these to link to?)
There was a problem hiding this comment.
Yes, of course :), at https://root.cern.ch/coding-conventions
There was a problem hiding this comment.
Thanks Philippe - Zhe, can you run through this guide and try to get the code to compliance?
@pcanal - when contributing to CVMFS, they provide an automated tool (based on cpplint). I see you reference astyle for managing whitespace. Do you have anything similarly automated you use?
|
Hi Zhe, You may also want to pull in the work done here: Specifically, Brian |
|
@bbockelm Sure. I will polish up my code. |
|
Hi Philippe, @pcanal Could you take a look at this new patch? Is there any test cases I can run with? Thanks! |
| { | ||
| const char *buffer = text; | ||
| size_t count = strlen(text); | ||
| ssize_t written = 0; |
There was a problem hiding this comment.
since written is not used outside the loop, it ought to be declared inside.
Not specifically. Some of the code would be exercised by calling ::Fatal( ... ) |
…ved initializations of shellExec and pidString into TUnixSystem::Init()
|
Hi, What was the result of your testing? Did you create a test we can add to roottest? Thanks, |
| typedef void (*SigHandler_t)(ESignals); | ||
|
|
||
| struct TStackTraceHelper { |
There was a problem hiding this comment.
Class not part of the public interface need to either be defined in an unnamed namepace in the source file or within the ROOT::Internal namespace or as protected/private subclass of the using class.
| return 0; | ||
| } | ||
|
|
||
| static int SignalSafeRead(int fd, char *inbuf, size_t len, int timeout=-1) { |
|
Since all the signal handling and processing is essentially standalone, could be move it all in a single (sub)class? Thanks, |
…per_t into TUnixSystem class
|
Hi Philippe, Talked with Zhe about this yesterday; now that the end-of-semester rush is over, he's coming back to this item. We're working to refactor this to a new class. Brian |
|
@pcanal @bbockelm I think there are two possible refactoring approaches.
Zhe |
|
I meant a variation of 1, where TSystem calls directly the extracted methods (i.e. essentially this should just be 'refactoring'). Thanks. |
|
Sure. It makes more sense to me. The only concern I have is how much side effect it will introduce to windows. I just ignore it from now? |
|
well, it would be no more or less than what you have now :). We will need to see how it behaves on Windows once we have merged it. |
|
@zzxuanyuan - since this appears to be replaced by #134, can you please close this request? |
@bbockelm @pcanal This patch copies the code of signal handling in CMSSW. It avoids async-unsafe functions in signal handler functions.
For reference, see the link https://github.com/bbockelm/cmssw/blob/stacktrace_handler_revisit/FWCore/Services/src/InitRootHandlers.cc
I tried this patch with some simple multi-thread test cases and it worked fine. Is there any complicated test cases I can run? I think this patch is not very ready to merge, but it achieved basic functions. Any criticisms are welcome.