delta: 🐛 Many delta processes being spawned, causing thrashing

$ git diff HEAD~ --stat
 Cargo.lock                                            |  15 +++++++++++++-
 library/alloc/tests/slice.rs                          | 120 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 library/backtrace                                     |   2 +-
 src/bootstrap/tool.rs                                 |   1 +
 src/doc/book                                          |   2 +-
 src/doc/edition-guide                                 |   2 +-
 src/doc/nomicon                                       |   2 +-
 src/doc/reference                                     |   2 +-
 src/doc/rust-by-example                               |   2 +-
 src/librustdoc/passes/collect_intra_doc_links.rs      |   3 +--
 src/llvm-project                                      |   2 +-
 src/test/ui/array-slice-vec/subslice-patterns-pass.rs | 126 ---------------------------------------------------------------------------------------------------------------
 src/tools/cargo                                       |   2 +-
 src/tools/miri                                        |   2 +-
 14 files changed, 145 insertions(+), 138 deletions(-)
$ PAGER= time -v git diff HEAD~ >/dev/null
	Command being timed: "git diff HEAD~"
	User time (seconds): 0.09
	System time (seconds): 0.18
	Percent of CPU this job got: 234%
	Elapsed (wall clock) time (h:mm:ss or m:ss): 0:00.11
	Average shared text size (kbytes): 0
	Average unshared data size (kbytes): 0
	Average stack size (kbytes): 0
	Average total size (kbytes): 0
	Maximum resident set size (kbytes): 33516
	Average resident set size (kbytes): 0
	Major (requiring I/O) page faults: 0
	Minor (reclaiming a frame) page faults: 9333
	Voluntary context switches: 131
	Involuntary context switches: 22
	Swaps: 0
	File system inputs: 0
	File system outputs: 0
	Socket messages sent: 0
	Socket messages received: 0
	Signals delivered: 0
	Page size (bytes): 4096
	Exit status: 0
$ PAGER=delta time -v git diff HEAD~
... still running as I write this issue ...

This is https://github.com/rust-lang/rust/commit/1bdee96c5e6de445f09df34447a42553294f21ed. ltrace shows that there are hundreds of delta processes running:

$ ps -ef | grep delta | wc -l
996

About this issue

Original URL
State: closed
Created 4 years ago
Comments: 16 (12 by maintainers)

Commits related to this issue

Don't prevent bat being used as pager Ref #378 — committed to dandavison/delta by dandavison 4 years ago
Document configuration via environment (#379) * [README,help] add warning : possible call cycle As described in the discussion of #378, certain constellations of envi- ronment variable configurat... — committed to dandavison/delta by Kr1ss-XD 4 years ago
Exit if it looks like starting the pager process would never terminate Fixes #529 Ref #378 — committed to dandavison/delta by dandavison 3 years ago
Exit if it looks like starting the pager process would never terminate Fixes #529 Ref #378 — committed to dandavison/delta by dandavison 3 years ago
Exit if it looks like starting the pager process would never terminate (#531) Fixes #529 Ref #378 — committed to dandavison/delta by dandavison 3 years ago
Silently change pager to less if $PAGER is delta Ref #531, #529, #378 — committed to dandavison/delta by dandavison 3 years ago

Most upvoted comments

This should be configured via git --config core.pager or $GIT_PAGER though. (not $PAGER)

Yes, I think I agree that this is technically the correct answer. Unlike bat, no-one will be using delta as a general purpose pager, so I’m thinking the recursion is really only a danger in the precise situation of this ticket – using environment variables to modify git’s pager. If someone accidentally has PAGER=delta in their shell config, they’re going to get some weird behavior sooner or later. Using PAGER rather than GIT_PAGER is certainly a natural thing to do when doing a timing experiment like in this ticket, but I think this problem is going to be sufficiently uncommon that the code doesn’t need to actually watch for it and override the pager executable like general-purpose pager-spawning pagers like bat do. Perhaps this is my tendency to err on the side of purism over pragmatism 😃 Anyway, this is all pretty minor. Thanks @jyn514 for the issues you opened; if people hit this recursion again I’ll stick in the protection.

dandavison on Nov 5, 2020

Haha! Beautiful. The delta fork bomb. (EDIT: ok, not really, this is linear) I was having trouble reproing that for a moment, but eventually realized that I had to unset BAT_PAGER and DELTA_PAGER in order for it to become recursive (relevant code).

Do either of you think a fix is needed? I’m kind of inclined to think that it’s in the spirit of Unix to allow people to do stuff like that, and that code in delta saying “Hey, judging by the executable name it looks like you’re about to create a cycle in your process call graph; I’m not going to allow that.” would be no fun (and not truly correct since who knows how a user has named their executables or whether they’ve arranged somehow for the cycle to be broken.)

dandavison on Nov 4, 2020