Page MenuHomePhabricator

Use "exec" to skip the Dash intermediate shell for "pygmentize"
ClosedPublic

Authored by epriestley on Jan 29 2019, 2:04 PM.

Details

Summary

See T13224. A user reports the setTimeout() not actually killing Pygments under Ubuntu, where the default shell is "dash".

I think what's happening here is:

  • Dash creates a strict subprocess always, Bash only does sometimes. If you "bash" a single simple command, it automatically behaves like you "exec"'d it and replaces itself.
  • So we end up with "dash" as our subprocess, and then "pygments" under it.
  • setTimeout() does SIGTERM + wait + SIGKILL.
  • When we SIGTERM, I'd guess it propagates to "pygments" but "pygments" ignores it (it's stuck inside regexp explosion land).
  • When we SIGKILL, I think it kills "dash" and leaves "pygments" alive.

I didn't really test any of this since it's theoretically sound and adding "exec" is generally safe, and the reporting user says it fixed things.

Test Plan

Highlighted some non-problematic code locally to make sure this didn't break anything in the working case.

Diff Detail

Repository
rPHU libphutil
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

epriestley created this revision.Jan 29 2019, 2:04 PM
epriestley requested review of this revision.Jan 29 2019, 2:05 PM
amckinley accepted this revision.Jan 30 2019, 6:25 AM
amckinley added inline comments.
src/markup/syntax/highlighter/PhutilPygmentsSyntaxHighlighter.php
25

I honestly thought "dash" was a typo in T13224.

This revision is now accepted and ready to land.Jan 30 2019, 6:25 AM
This revision was automatically updated to reflect the committed changes.