https://pantsbuild.org/ logo
Powered by
h

hundreds-breakfast-49010

09/14/2020, 7:56 PM
does anyone know (at a high level) what 
reset_log_location
https://github.com/pantsbuild/pants/blob/master/src/python/pants/base/exception_sink.py#L144 was meant to do at a high level?
the docstring says "Re-acquire file handles to error logs based in the new location.", but I'm not sure what the new location its talking about is; or in general to what extent this is out of date compared to how pants logging works today in a rust context
h

hundreds-father-404

09/14/2020, 7:58 PM
@aloof-angle-91616 do you remember? https://github.com/pantsbuild/pants/pull/6552 looks relevant
It’s 2 year old code. And the Rust logging is ~1 year old thanks to Borja’s changes. So it sounds very plausible that it is out of date
h

hundreds-breakfast-49010

09/14/2020, 7:59 PM
it looks like this code is responsible for creating the 
.pids
directory in the workdir
which I haven't generally looked at
was that directory called 
.pids
because it want to have thread-specific debug files back when pantsd was multithreaded?
h

hundreds-father-404

09/14/2020, 8:03 PM
That I don’t know. I know that generally there was a reason to not have 
.pids
as a part of 
.pants.d
because of the logic with cleaning 
.pants.d
, iirc. https://github.com/pantsbuild/pants/issues/3511 might have some helpful context
h

hundreds-breakfast-49010

09/14/2020, 8:05 PM
yeah I think there's a broader question of what behavior we actually want out of logging to 
.pids
w

witty-crayon-22786

09/14/2020, 8:43 PM
signal handlers have constraints on what they are able to do
one thing that they can’t do is open files
so to interact with a file in a signal handler, you need to already have a file handle for it open
h

hundreds-breakfast-49010

09/14/2020, 8:48 PM
@witty-crayon-22786 so ExceptionSink should be keeping around handles to open files, so signal handlers can write to them?
this is the 
SignalHandledNonLocalExit
mechanism I think?
w

witty-crayon-22786

09/14/2020, 8:49 PM
this is the 
SignalHandledNonLocalExit
 mechanism I think?
maybe, i don’t know.
h

hundreds-breakfast-49010

09/14/2020, 8:50 PM
anyway we call 
reset_log_location
exactly once when we start pants, and never invoke it again
I don't know if that's always been the case
w

witty-crayon-22786

09/14/2020, 8:50 PM
it used to have more calls i think.
h

hundreds-breakfast-49010

09/14/2020, 8:51 PM
well actually no, we call it once in pants_runner, once in pants_daemon, and once in the exception sink module itself
that last one might be redundant
w

witty-crayon-22786

09/14/2020, 8:51 PM
i don’t think it is… it needs to be initialized
h

hundreds-breakfast-49010

09/14/2020, 8:56 PM
anyway the reason i was looking at this was because the test that's sporadically failing in https://github.com/pantsbuild/pants/pull/10758 is one of the ones that was meant to test the 
.pids
logging functionality, I believe
I was trying to see if we could just get rid of that test, but I think it's testing functionality we do actually want
Powered by