Skip to content

Conversation

takase1121
Copy link
Member

@takase1121 takase1121 commented Jan 30, 2023

This fixes a weird issue where when any of the child process syscalls fails, errors are not sent back to parent but is instead sent to stderr. This is not the case with previous reproc-based API and can be hard to debug.

Squash me.

@adamharrison
Copy link
Member

Hrm. The idea that it's written to stderr makes sense though, as the parent process would be listening on that pipe. Whether the exec call itself fails, or the process immediately exits with some other error; the process to determine the error would be the same: i.e. to read the stderr pipe.

What's the benefit of splitting this off to it's own pipe with distinct error handling?

@adamharrison
Copy link
Member

adamharrison commented Jan 30, 2023

Actually, never mind, I can see why this'd be useful. Would eliminate an easy class of errors a lot more effectively; and would harmonize with windows behaviour. OK. Will review tomorrow.

@takase1121
Copy link
Member Author

takase1121 commented Jan 30, 2023

Hrm. The idea that it's written to stderr makes sense though, as the parent process would be listening on that pipe. Whether the exec call itself fails, or the process immediately exits with some other error; the process to determine the error would be the same: i.e. to read the stderr pipe.

What's the benefit of splitting this off to it's own pipe with distinct error handling?

There are a few issues:

  1. this behavior was not made clear in docs since it was introduced, so I assume it's a bug
  2. it requires a read right after process start, so its a little bit less intuitive since exec errors almost always seems instantaneous
  3. any process could just as easily return -1 as exit code and users can't differentiate it.
  4. its not the same on Windows

@adamharrison
Copy link
Member

adamharrison commented Jan 31, 2023

  1. this behavior was not made clear in docs since it was introduced, so I assume it's a bug
  2. it requires a read right after process start, so its a little bit less intuitive since exec errors almost always seems instantaneous
  3. any process could just as easily return -1 as exit code and users can't differentiate it.
  4. its not the same on Windows

Yes, this parallels my understanding. Reviewing now.

@adamharrison
Copy link
Member

Tested this; worked perfectly. Merging.

@adamharrison adamharrison merged commit 9d7a9ac into lite-xl:master Jan 31, 2023
takase1121 added a commit to takase1121/lite-xl that referenced this pull request Aug 19, 2023
* fix: exec() error not returned to parent

* chore: remove accidental lua.h inclusion
takase1121 added a commit to takase1121/lite-xl that referenced this pull request Aug 19, 2023
* fix: exec() error not returned to parent

* chore: remove accidental lua.h inclusion
@takase1121 takase1121 deleted the PR/exec-error branch March 23, 2025 04:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants