search

wulkano / Aperture

Record the screen on macOS
MIT License
1.23k stars 111 forks source link

Exit process on request #35

Closed albinekb closed 6 years ago

albinekb commented 7 years ago

If you run node example.js and ctrl+c (quit) before recorder.stopRecording() is called, the swift process won't be killed.

I think that's because execa kills it with the default kill code SIGTERM

which is catched here: https://github.com/wulkano/aperture/compare/master...albinekb:exit-code?expand=1#diff-f4030c3d0a3c8d0746ab9fdabb019bebL48

and because that handler is there, it won't quit.

With this change it ensures that the process will always die 👍

sindresorhus commented 7 years ago

Hmm, strange. It should be working already as recorder.stop() causes the recording to stop. When the recording is stopped, https://github.com/wulkano/aperture/blob/d63e7ec718563b7d4c49bd70798fe6ebd4010fed/swift/aperture/Recorder.swift#L81 is called, which calls onFinish?(), which calls https://github.com/wulkano/aperture/blob/d63e7ec718563b7d4c49bd70798fe6ebd4010fed/swift/aperture/main.swift#L38

Any idea why that is not working?

albinekb commented 7 years ago

Hmm. No idea, I can try to look into it a bit more to see if I can see what's going on

sindresorhus commented 6 years ago

@albinekb What should we do about this?

albinekb commented 6 years ago

Maybe onFinish doesn't get called after the exit code is sent? The only working solutions I can think about is:

  1. Change kill code from node (execa) to something like SIGHUP?
  2. Apply this PR to make sure it quits
  3. Investigate more? (I don't really have time to do that right now..)

Is there any downsides on adding this exit(0) except for it not being super elegant?

albinekb commented 6 years ago

ping @sindresorhus should we take 1 or 2 and fix it or just close it and let it hang? 😄

sindresorhus commented 6 years ago

I'm ok with landing this PR, but can you add an inline code comment referring to this PR, so I don't accidentally remove the exit call in the future.

albinekb commented 6 years ago

Will do 👍

albinekb commented 6 years ago

@sindresorhus sorry for the delay, I have added the comment 👍

albinekb commented 6 years ago

Thanks!

sindresorhus commented 6 years ago

I had to revert this as it caused random corruptions which were really difficult to track down.

https://github.com/wulkano/aperture/commit/8c98ed57dbd6add062274c34a6d74404f9f639d6