ARROW-4996: [Plasma] Enable uninstalling of signal handler and fix log_dir

[guoyuhong]

1. UninstallSignalAction is added at exiting time.
2. When log_dir is empty, we should not call InitGoogleLogging, which means there should be not log files. If log_dir is not empty, we should call SetLogDestination for different log levels.

[fsaintjacques]

Why do you need to de-register the signal handler?

[fsaintjacques]

Do both variables need to be kept publicly? They could be private static in InstallFailureSignalHandler.

[guoyuhong]

Will change it for app_name_, but for log_dir_, this variable will be used in 2 functions. It is better to keep it as a static variable.

[fsaintjacques]

but it doesn't need to be public, you can move it into the implementation (logging.cc).

[guoyuhong]

Will do.

[guoyuhong]

@fsaintjacques Thanks for the comment!

[guoyuhong]

@fsaintjacques Do you have more comments on this PR？

[fsaintjacques]

good for me.

[guoyuhong]

In some rare cases, when plasma is exiting in abnormal cases, glog seems to fall into a dead-lock state. When we de-register the signal handler, the problem is gone.

[guoyuhong]

@pitrou Could you take a look?

[pitrou]

I fail to understand why this solves the "There are many duplicated log files in /tmp" issue. Can you explain?

[guoyuhong]

@pitrou We pass empty log_dir_ to StartArrowLog and we expect that log only exists in stderr(we can redirect it). We found there is duplicated log in /tmp. This is because we call google::InitGoogleLogging in this function but not call SetLogDestination for all other levels.

[fsaintjacques]

There's a big warning on deadlock about using glog's signal handler feature and linux 64-bits. https://github.com/google/glog/blob/6deff5ab23be84f8fb5cd434905eb3672d95c82f/INSTALL#L13-L67

Usually, libraries should not install signal handlers, only applications (plasma client/server).

[guoyuhong]

@fsaintjacques Yes. We may meet the dead log problem and after we de-register the signal handler this problem is gone. Yes, currently, only Plasma_Store calls InstallFailureSignalHandler. The lib does not call it.

[pitrou]

Thanks @guoyuhong. I will wait for CI and then merge.

Forgot something

[pitrou]

Oops, sorry... I missed something. If only Plasma store registers the signal handlers, then only Plasma store should unregister the signal handlers. Especially as the method sets them to SIG_DFL rather than restore the actual original value.

[codecov-io]

Codecov Report

Merging #4013 into master will increase coverage by 0.8%.
The diff coverage is 100%.

@@            Coverage Diff            @@
##           master    #4013     +/-   ##
=========================================
+ Coverage   87.85%   88.65%   +0.8%     
=========================================
  Files         736      602    -134     
  Lines       90595    80975   -9620     
  Branches     1252        0   -1252     
=========================================
- Hits        79594    71791   -7803     
+ Misses      10884     9184   -1700     
+ Partials      117        0    -117

Impacted Files	Coverage Δ
cpp/src/arrow/util/logging.h	60% <ø> (ø)	⬆️
cpp/src/arrow/util/logging.cc	90.66% <100%> (+5.92%)	⬆️
cpp/src/arrow/io/readahead.cc	95.95% <0%> (-1.02%)	⬇️
cpp/src/arrow/util/bpacking.h	99.81% <0%> (-0.01%)	⬇️
go/arrow/math/uint64_amd64.go
go/arrow/memory/memory_avx2_amd64.go
js/src/enum.ts
go/arrow/array/builder.go
js/src/Arrow.node.ts
js/src/schema.ts
... and 129 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6008e43...ddec3f7. Read the comment docs.

[guoyuhong]

@pitrou Do you mean that we should separate the function UninstallSignalAction from ShutDownArrowLog and only call it in Plasma Store?

[pitrou]

@guoyuhong Right.

[guoyuhong]

@pitrou I have moved UninstallSignalAction from ShutDownArrowLog to plasma store.

[pitrou]

+1 again