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

Raise exception when waitpid would block. #2222

Closed
wants to merge 1 commit into from

Conversation

Projects
None yet
3 participants
@toots
Copy link
Contributor

commented Jan 17, 2019

This PR handles the case when waitpid is called with WNOHANG and returns 0 for the pid. In this case, process status is undefined but the C binding code currently will always return a WSIGNALED type with undefined parameter. This PR make it so it raises an exception instead.

The windows case isn't yet covered. It seems that the windows C does not return a tagged type at all but just the process's exit code. Is that intended? That seems like a bug as well.

@toots toots force-pushed the toots:waitpid-nohang branch from 53acdb6 to 4ef7040 Jan 17, 2019

@toots toots force-pushed the toots:waitpid-nohang branch from 4ef7040 to 93d916f Jan 17, 2019

@stedolan

This comment has been minimized.

Copy link
Contributor

commented Jan 18, 2019

I agree that the current interface isn't great, and something based on exceptions or option types might have been a better API. But this change will break every single use of WNOHANG, which is widely used.

Your alternative API might fit better as a separate function or even in a separate library. It's easy to implement using the built-in Unix.waitpid:

match Unix.waitpid ... with
| 0, _ -> raise Waitpid_would_block
| s -> s
@dra27

This comment has been minimized.

Copy link
Contributor

commented Jan 18, 2019

There's no bug in the Windows version - win_waitpid only ever returns WEXITED so the tag is always 0.

@dra27

This comment has been minimized.

Copy link
Contributor

commented Jan 18, 2019

As @stedolan points out, we can't change the behaviour of the existing function. I'm not initially convinced of the value in an extra function in the Unix module for this, but feel free to re-open with revised code. What's your motivation for the change - "just" philosophical, or has the behaviour caused an issue?

@dra27 dra27 closed this Jan 18, 2019

@toots

This comment has been minimized.

Copy link
Contributor Author

commented Jan 18, 2019

Cool, I understand. My motivation is that I don't think it's safe to have code manipulating a low-level C value that is explicitly documented as undefined in this case:

static value alloc_process_status(int pid, int status)
{
  value st, res;

  if (WIFEXITED(status)) {
    st = caml_alloc_small(1, TAG_WEXITED);
    Field(st, 0) = Val_int(WEXITSTATUS(status));
  }
  else if (WIFSTOPPED(status)) {
    st = caml_alloc_small(1, TAG_WSTOPPED);
    Field(st, 0) = Val_int(caml_rev_convert_signal_number(WSTOPSIG(status)));
  }
  else {
    st = caml_alloc_small(1, TAG_WSIGNALED);
    Field(st, 0) = Val_int(caml_rev_convert_signal_number(WTERMSIG(status)));
  }
  Begin_root (st);
    res = caml_alloc_small(2, 0);
    Field(res, 0) = Val_int(pid);
    Field(res, 1) = st;
  End_roots();
  return res;
}

In this function, status is undefined when pid is 0. The behavior of these macros and conversions functions thus also is. What if WTERMSIG(status), for instance, was to cause a segfault in this case in the future? At the very least this function should not be touching status in this situation.

@stedolan

This comment has been minimized.

Copy link
Contributor

commented Jan 18, 2019

Ah, I understand your concern now! I don't think we can or should change the interface to Unix.waitpid, but it would be good to avoid manipulating an undefined status value.

I suggest replacing the code:

  if (pid == 0)
    caml_raise_constant(*caml_named_value("Unix.Waitpid_would_block"));

with:

  if (pid == 0) status = 0;

0 is explicitly documented as a valid status result by the POSIX specification and corresponds to WIFEXITED with WEXITSTATUS(status) == 0.

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.