Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Change atom.sh not to spawn a sleep process every second on --wait #17403

Merged
merged 2 commits into from May 29, 2018

Conversation

Projects
None yet
3 participants
@lllusion3469
Copy link
Contributor

lllusion3469 commented May 24, 2018

Description of the Change

As sleep(1) is not a bash builtin, every second a new process is spawed.
To prevent this, the POSIX read can be used instead.
Since it is (required to be) a bash builtin, it is immediately killed along with bash unlike a longer running sleep would be.
In case stdin is e.g. /dev/null for whatever reason (this would break EDITOR=nano), sleep is still kept to prevent a tight loop.

Alternate Designs

  • a longer sleep interval: this would require job management as sleep would keep running even if bash is killed and thus cause huge delays

Why Should This Be In Core?

not possible otherwise

Benefits

Those sleep processes can be annoying in e.g. execsnoop or even htop

Possible Drawbacks

e: does consume stdin, though I don't see how that is a problem

Verification Process

  • edited my distributions /usr/bin/atom accordingly (Archlinux, Atom 1.27) (this file hasn't changed since)
  • opened a file using /usr/bin/atom --wait file and verified that it exited upon closing the tab

Applicable Issues

Change atom.sh not to spawn a sleep process every second on --wait
As sleep(1) is not a bash builtin, every second a new process is spawed.
To  prevent this, the POSIX read can be used instead.
Since it is (required to be) a bash builtin, it is immediately killed along with bash unlike a longer running sleep would be.
In case stdin is e.g. /dev/null for whatever reason (this would break EDITOR=nano), sleep is still kept to prevent a tight loop.
@maxbrunsfeld

This comment has been minimized.

Copy link
Contributor

maxbrunsfeld commented May 29, 2018

As sleep(1) is not a bash builtin, every second a new process is spawned.

Great point; thanks for bringing this up.

Rather than reading from stdin, what do you think about actually creating a pipe with mkfifo and reading from that?

Use a named pipe instead of the tty
Works even if stdin is not a terminal.

Some programs can replace a fifo with a named pipe, which would break this.
For example: rsync without --specials
sleep 1 # prevent a tight loop
done

# fall back to sleep

This comment has been minimized.

@maxbrunsfeld

maxbrunsfeld May 29, 2018

Contributor

Can you explain why this second "fall back" while loop is still needed?

If the read completes (presumably due to an error), won't we sleep and then recreate the pipe? I would think that we would already "fall back" to sleep, even without the second while loop.

This comment has been minimized.

@lllusion3469

lllusion3469 May 29, 2018

Author Contributor

If we (for some reason) can't create the fifo or remove the file that's there in its place, I guess.

If you want, I can remove it, though.
e: I agree that it doesn't make too much sense

This comment has been minimized.

@maxbrunsfeld

maxbrunsfeld May 29, 2018

Contributor

My knowledge of bash is somewhat limited, so I might be missing something.

I would think that it'd be impossible to reach that second while loop, because the while loop above would never complete. Is that true?

This comment has been minimized.

@lllusion3469

lllusion3469 May 29, 2018

Author Contributor

There's a break after the read

This comment has been minimized.

@maxbrunsfeld

maxbrunsfeld May 29, 2018

Contributor

Oh, thanks for explaining; I didn't notice that.

So why is the read wrapped in a while loop? In what circumstance would read exit successfully?

This comment has been minimized.

@lllusion3469

lllusion3469 May 29, 2018

Author Contributor

For example if you write to the FIFO.
I don't know why you would, again, but it's possible.

This comment has been minimized.

@maxbrunsfeld

maxbrunsfeld May 29, 2018

Contributor

I see, so in that case it should be fine to read again rather than falling back to a sleep, but we don't want to retry if there appears to be a problem with the fifo. That actually makes perfect sense.

@maxbrunsfeld maxbrunsfeld merged commit ac840d9 into atom:master May 29, 2018

2 of 3 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@lllusion3469 lllusion3469 deleted the lllusion3469:patch-1 branch May 29, 2018

@maxbrunsfeld

This comment has been minimized.

Copy link
Contributor

maxbrunsfeld commented Jun 5, 2018

Another benefit of this PR is that the atom.sh process exits instantly after you close the waited buffers, instead of waiting for up to one second for the sleep to complete.

@MartinBonner

This comment has been minimized.

Copy link

MartinBonner commented Nov 23, 2018

Version 1.28.2 of atom (which doesn't have this change) works fine as my git commit editor. Version 1.29.0 does not - git commit hangs indefinitely waiting for the editor to finish. I am not convinced this change is what causes the problem - but it does seem the most likely point to start looking.

What diagnostics would be useful to identify what is causing the problem?

@lllusion3469

This comment has been minimized.

Copy link
Contributor Author

lllusion3469 commented Nov 23, 2018

  • What's your operating system?
  • What are your EDITOR/VISUAL/GIT_EDITOR environment variables / core.editor git setting?
  • How did you install Atom?

Also, belated thanks to @maxbrunsfeld for fixing my mistake completely breaking platforms other than linux.

@MartinBonner

This comment has been minimized.

Copy link

MartinBonner commented Nov 26, 2018

Sorry for the belated reply. The problem is on my work computer, and the weekend got in the way.

Operating system is Ubuntu 14.04. uname -a says:

Linux [redacted] 4.4.0-139-generic #165~14.04.1-Ubuntu SMP Wed Oct 31 10:55:11 UTC 2018 x86_64 x86_64 x86_64 GNU/Linux

None of the environment variables are set. core.editor is atom --wait

I installed atom with sudo apt-get atom=1.28.2. dpkg -s shows:

Package: atom
Status: install ok installed
Priority: optional
Section: devel
Installed-Size: 562156
Maintainer: GitHub <atom@github.com>
Architecture: amd64
Version: 1.28.2
Depends: git, gconf2, gconf-service, libgtk2.0-0, libudev0 | libudev1, libgcrypt11 | libgcrypt20, libnotify4, libxtst6, libnss3 (>= 2:3.22), python, gvfs-bin, xdg-utils, libcap2, libx11-xcb1, libxss1, libasound2 (>= 1.0.16), libxkbfile1
Recommends: lsb-release
Suggests: libsecret-1-0, gir1.2-gnomekeyring-1.0
Description: A hackable text editor for the 21st Century.
 Atom is a free and open source text editor that is modern, approachable, and hackable to the core.

apt-cache policy atom suggests I am getting the package from https://packagecloud.io/AtomEditor/atom/any/

I can get you the corresponding output for atom 1.29.0 if that would be helpful.

@MartinBonner

This comment has been minimized.

Copy link

MartinBonner commented Dec 2, 2018

@lllusion3469

This comment has been minimized.

Copy link
Contributor Author

lllusion3469 commented Dec 2, 2018

Edited version of my deleted comment:

I finally came around to installing Ubuntu 14.04 in a VM and I can indeed replicate this with Atom 1.29.0...
Fiddling around with /usr/bin/atom seems to indicate that this PR caused it:

Removing read < "$WAIT_FIFO" fixes it

though it kind of goes against the point of this PR

and so does read <> "$WAIT_FIFO".

the version I would prefer, though I don't know why it works.

It does not, in fact, seem to have anything to do with bash:
Even with a manually compiled (just make install) bash on master branch it doesn't work on Ubuntu 14.04.
I wasn't running bash properly - make install installs it at /usr/local/bin/bash, not /bin/bash 🤦

I don't really want to create a PR to fix this as long as I don't know what causes this.

@lllusion3469

This comment has been minimized.

Copy link
Contributor Author

lllusion3469 commented Dec 2, 2018

I still don't get why this is happening, but:

  1. read < "$WAIT_FIFO" blocks, not because of read(2), but at the open(2) syscall before calling read, so any builtin could be used, even true and the result would be the same
  2. with read <> "$WAIT_FIFO" it is important that read is used because here it is read(2) that blocks since now the FIFO can actually be opened (requires a reader and a writer)
  3. on Ubuntu 14.04, for some reason, (only) with read < "$WAIT_FIFO" the signal handler doesn't get called and the program resumes where it stopped - at open(2)
@lllusion3469

This comment has been minimized.

Copy link
Contributor Author

lllusion3469 commented Dec 4, 2018

It's this bash commit that fixed it

Edit: and bash 4.3 that broke it, though it seems they weren't using git back then.

@lllusion3469

This comment has been minimized.

Copy link
Contributor Author

lllusion3469 commented Dec 5, 2018

As Ubuntu 14.04 is still supported, you might have success contacting them (though I'm not too familiar with their rules) as it's clearly a bug in bash (even dash handles this correctly), that's even been fixed upstream.

Though it might still be appropriate to implement a simple workaround (e.g. read -r arbitrary_variable <> "$WAIT_FIFO")

Edit: also here's a simpler script demonstrating the problem:

#!/usr/bin/env bash
trap 'echo trap; exit 0' TERM
echo "PID: $$"
true < /tmp/fifotest
  1. save this script e.g. in a.sh
  2. create a FIFO: mkfifo /tmp/fifotest
  3. call the bash script: bash a.sh
  4. kill the PID it prints out in another terminal using kill -TERM $PID (replacing $PID)
  5. it should print out trap and then exit
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.