fix hang in git push when pack-objects fails

The wait_or_whine function will complain to stderr in a few cases:

1. We fail to actually waitpid() correctly.

2. The child died of a signal.

3. The child returned exit code 127, indicating a missing command to exec after forking.

We already have a silent_exec_failure flag to silence (3). Let's convert that into a "quiet" flag to also silence (2).

This shouldn't result in signal failure being silent for existing users of silent_exec_failure, since they already will need to be checking the return code and complaining for the case of a non-zero exit code.

Quoted 7 lines · Jeff King

The wait_or_whine function will complain to stderr in a few cases:

1. We fail to actually waitpid() correctly.

2. The child died of a signal.

Quoted 5 lines

3. The child returned exit code 127, indicating a missing command to exec after forking.

We already have a silent_exec_failure flag to silence (3). Let's convert that into a "quiet" flag to also silence (2).

I'm rather negative on controlling these two error reports with the same flag because...

We provide only two abstract promitives for async code:
start and finish. Where "finish" means to wait until the async code tells us it is done. However, it may also be useful for us to to tell the async code to abort right away, because whatever it was doing is no longer useful.

For a separate process, we just kill() it. For Windows, we need to do whatever the equivalent to pthread_cancel is.

Signed-off-by Jeff King <[email protected]>

---
 run-command.c |   10 ++++++++++
 run-command.h |    1 +

 2 files changed, 11 insertions(+), 0 deletions(-)

run-command.c +0 -0

index 258c880..f179d2a 100644

Quoted 8 lines

run-command.c +0 -0

index 258c880..f179d2a 100644
@@ -439,6 +439,16 @@ int finish_async(struct async *async)
        return ret;
  }
 

run-command.c int finish_async(struct async *async) · line 445

Quoted 4 lines

run-command.c +4 -0

+void abort_async(struct async *async)
+{
+#ifndef WIN32
+       kill(async->pid, 15);

This doesn't compile unless NO_PTHREADS is set, no?

This should probably be

void abort_async(struct async *async)
{

#ifdef NO_PTHREADS
	kill(async->pid, 15);
#else
	pthread_cancel(async->tid)
#endif
	finish_async(async);
}

... and then us Windows-guys must implement something like pthread_cancel().

Or maybe not. Can pthread reliably cancel a thread while making sure that thread isn't holding a mutex (like some thread-safe malloc implementations do)? If not, I'm not entirely sure we can even reach this goal.

If pack-objects dies prematurely (for example, because there is some repo corruption), we are careful to clean up the sideband demuxer (if it is being used) with finish_async. However, for an async implementation which forks (i.e., not Windows), that means we will waitpid() for the async process.

Meanwhile, the async sideband demuxer will continue trying to stream data from the remote repo until it gets EOF. Depending on what data pack-objects actually sent, the remote repo may not actually send anything (e.g., if we sent nothing and it is simply waiting for the pack). This leads to deadlock cycle in which send-pack waits on the demuxer, the demuxer waits on the remote receive-pack, and the remote receive-pack waits on send-pack to send the pack data.

Quoted 8 lines

Meanwhile, the async sideband demuxer will continue trying to stream data from the remote repo until it gets EOF. Depending on what data pack-objects actually sent, the remote repo may not actually send anything (e.g., if we sent nothing and it is simply waiting for the pack). This leads to deadlock cycle in which send-pack waits on the demuxer, the demuxer waits on the remote receive-pack, and the remote receive-pack waits on send-pack to send the pack data.

This is an indication that a writable end of the pipe between send-pack and receive-pack is still open. This fixes the deadlock for me without having to kill the demuxer explicitly:

builtin/send-pack.c +3 -0

index 5e772c7..db32ded 100644
@@ -229,6 +229,9 @@ static void print_helper_status(struct ref *ref)
 static int sideband_demux(int in, int out, void *data)
 {
 	int *fd = data;
+#ifdef NO_PTHREADS
+	close(fd[1]);
+#endif
 	int ret = recv_sideband("send-pack", fd[0], out);
 	close(out);
 	return ret;

If only I had a brilliant idea how to forge this into a re-usable pattern...

Obviously it totally breaks the start_async abstraction if the called code needs to care whether it forked or not. But we can use that to our advantage, since it means start_async callers must assume the interface is very limited. So I think we can do something like:

1. Async code declares which file descriptors it cares about. This would automatically include the pipe we give to it, of course. So the declared ones for a sideband demuxer would be stderr, and some network fd for reading.

2. In the pthreads case, we do nothing. In the forked case, the child closes every descriptor except the "interesting" ones.

And that solves this problem, and the general case that async-callers have no idea if they have just leaked pipe descriptors in the forked case.

I'm still slightly confused, though, because I never see that descriptor get closed in the threaded case. So I still don't understand why it _doesn't_ deadlock with pthreads.

Quoted 13 lines · Jeff King

Obviously it totally breaks the start_async abstraction if the called code needs to care whether it forked or not. But we can use that to our advantage, since it means start_async callers must assume the interface is very limited. So I think we can do something like:

1. Async code declares which file descriptors it cares about. This would automatically include the pipe we give to it, of course. So the declared ones for a sideband demuxer would be stderr, and some network fd for reading.

2. In the pthreads case, we do nothing. In the forked case, the child closes every descriptor except the "interesting" ones.

Quoted 3 lines

And that solves this problem, and the general case that async-callers have no idea if they have just leaked pipe descriptors in the forked case.

Sounds like a plan. How do you close all file descriptors? Just iterate up to getrlimit(RLIMIT_NOFILE)?

Quoted 3 lines

I'm still slightly confused, though, because I never see that descriptor get closed in the threaded case. So I still don't understand why it _doesn't_ deadlock with pthreads.

In the threaded case, this fd is closed by start_command(), where it is passed as po.out in pack_objects(). In the fork case this is too late because a duplicate was already inherited to the sideband demuxer.

However, pack_objects() works differently in the stateless_rpc case: then it does not close fd anywhere, and I think it should be possible to construct a similar case that hangs even in the threaded case. And the fix could simply look like this:

builtin/send-pack.c +0 -0

index 5e772c7..c8f601f 100644

Quoted 14 lines · Johannes Sixt

Quoted 13 lines · Jeff King

Obviously it totally breaks the start_async abstraction if the called code needs to care whether it forked or not. But we can use that to our advantage, since it means start_async callers must assume the interface is very limited. So I think we can do something like:

1. Async code declares which file descriptors it cares about. This would automatically include the pipe we give to it, of course. So the declared ones for a sideband demuxer would be stderr, and some network fd for reading.

2. In the pthreads case, we do nothing. In the forked case, the child closes every descriptor except the "interesting" ones.

Quoted 6 lines

Quoted 3 lines

And that solves this problem, and the general case that async-callers have no idea if they have just leaked pipe descriptors in the forked case.

Sounds like a plan. How do you close all file descriptors? Just iterate up to getrlimit(RLIMIT_NOFILE)?

Sadly, yes, I think that is what we would have to do. It does feel like an awful hack. And it will interact badly with things like valgrind, which open descriptors behind the scenes (but can properly handle the forking).

I just don't see another way around it for the general case. The "usual" fix for this sort of thing is that the descriptors should have close-on-exec set, but that doesn't work for us here, because we are only forking.

It's sufficiently ugly (and still possible to break in the pthreads case!) that it may be worth not worrying about the general case at all, and just fixing this one with the explicit close.

Quoted 4 lines

Quoted 3 lines

I'm still slightly confused, though, because I never see that descriptor get closed in the threaded case. So I still don't understand why it _doesn't_ deadlock with pthreads.

Quoted 3 lines

In the threaded case, this fd is closed by start_command(), where it is passed as po.out in pack_objects(). In the fork case this is too late because a duplicate was already inherited to the sideband demuxer.

Hrm, I see the code now. That seems like an odd thing to do to me.

Doesn't it disallow:

  /* set up a command */
  const char **argv = { "some", "command" };
  struct child_process c;
  c.argv = argv;
  c.out = fd;

  /* run it */
  run_command(&c);

/* now tack our own output to the end */
write(fd, "foo", 3);

And even weirder, we only do the close for high file descriptors. So you _can_ do that above if "fd" is stdout, but not with an arbitrary fd.

Quoted 5 lines

Quoted 3 lines

In the threaded case, this fd is closed by start_command(), where it is passed as po.out in pack_objects(). In the fork case this is too late because a duplicate was already inherited to the sideband demuxer.

Hrm, I see the code now. That seems like an odd thing to do to me.

Why so? It's a matter of resource ownership: If you pass a positive value, you give away ownership; if you pass -1, you gain ownership; if you pass 0, ownership remains unchanged.

Quoted 8 lines

Doesn't it disallow:

  /* set up a command */
  const char **argv = { "some", "command" };
  struct child_process c;
  c.argv = argv;
  c.out = fd;

Quoted 5 lines

  /* run it */
  run_command(&c);

/* now tack our own output to the end */
write(fd, "foo", 3);

You would have to dup() the fd before run_command().

Quoted 2 lines

And even weirder, we only do the close for high file descriptors. So you _can_ do that above if "fd" is stdout, but not with an arbitrary fd.

Ah, right, that's a bit dubious. The reason is that if you want to tell the child process to use the parent's stdout for its own stdout, you specify 0 aka "no special treatement", i.e. just inherit from the parent, not 1. IOW, 1 is never a sane candidate to be assigned to c.out.

So what do you want to do about the fd that needs closing? The options I see are:

1. Try for a general solution. That probably means the "close every descriptor in the child" hackiness that I mentioned earlier.

2. Fix this case by having the async code close it if it was forked. It needs to know whether we forked, so we can:

a. Use NO_PTHREADS. Easy and simple, though it does break
start_async's abstraction a bit.

b. Have start_async pass in a flag telling what happened. This
really breaks the abstraction very similarly to (a), but it makes the connection more explicit.

I think I am leaning a bit towards (2a). It's simple, and it's not like this is library code with a million unknown callers; fixing it simply and cleanly with a nice commit message is probably sufficient.

I think I am leaning a bit towards (2a). It's simple, and it's not like this is library code with a million unknown callers; fixing it simply and cleanly with a nice commit message is probably sufficient.

Quoted 3 lines · Jeff King

I think I am leaning a bit towards (2a). It's simple, and it's not like this is library code with a million unknown callers; fixing it simply and cleanly with a nice commit message is probably sufficient.

This really sounds like a plan. Even if we _might_ later want to go to 2b. or some other solution, we will know what pattern to grep for.

In the non-stateless-rpc case, the writable end of the channel to the remote repo is closed by the start_command() call that runs the pack-objects process (after pack-objects inherited a copy). But in the --stateless-rpc case, where send-pack takes care of writing data to the channel, this was missed.

[...]

Warning: This patch is untested. Furthermore, it does not even fix a resource leak because the fd that is now closed in pack_objects() would be closed later in cmd_send_pack.

However, without closing the fd earlier like this, a --stateless-rpc invocation could theoretically dead-lock just like a regular invocation in a NO_PTHREADS build. But I also don't know how to test-drive send-pack --stateless-rpc to construct such a case. Any hints how to do that would be appreciated.

builtin-send-pack.c static void print_helper_status(struct ref *ref) · line 381

Quoted 12 lines

builtin-send-pack.c +3 -0

index 089058b..b371c79 100644
@@ -376,6 +376,9 @@ static void print_helper_status(struct ref *ref)
 static int sideband_demux(int in, int out, void *data)
 {
 	int *fd = data;
+#ifndef ASYNC_AS_THREAD
+	close(fd[1]);
+#endif

In the comments for 1/2, you said this goes directly on 38a81b4e. But in that commit, we use #ifndef WIN32 to decide whether or not to fork for async code. So shouldn't this use the same test (I don't even see ASYNC_AS_THREAD defined anywhere else)?

... Therefore, I assumed that we use the
former symbol to decide whether to use threaded async procedures. Obviously, the symbol was introduced only later. Will resend.

In the comments for 1/2, you said this goes directly on 38a81b4e. But in that commit, we use #ifndef WIN32 to decide whether or not to fork for async code. So shouldn't this use the same test (I don't even see ASYNC_AS_THREAD defined anywhere else)?

Quoted 4 lines

In the comments for 1/2, you said this goes directly on 38a81b4e. But in that commit, we use #ifndef WIN32 to decide whether or not to fork for async code. So shouldn't this use the same test (I don't even see ASYNC_AS_THREAD defined anywhere else)?

Here's the fixed patch. I squashed both earlier patches into a single patch because they are about the same topic, as you showed with your tests of git-push via smart http.

Again, this should go on top of 38a81b4e. When it is merged to f6b60983 or later, the '#ifndef WIN32' must be changed to '#ifdef NO_PTHREADS'.

We just need to cancel the thread, instead of sending it a signal as we do for fork()'d async sections.

Signed-off-by Jeff King <[email protected]>

---
This could also just be part of the merge resolution, but I thought it would be easier to see what is going on if I put it here.

 run-command.c |    2 +-

 1 files changed, 1 insertions(+), 1 deletions(-)

run-command.c +0 -0

index e31c073..46ea07d 100644

Quoted 11 lines · Jeff King

We just need to cancel the thread, instead of sending it a signal as we do for fork()'d async sections.

Signed-off-by Jeff King <[email protected]>

---
This could also just be part of the merge resolution, but I thought it would be easier to see what is going on if I put it here.

 run-command.c |    2 +-

 1 files changed, 1 insertions(+), 1 deletions(-)

run-command.c void abort_async(struct async *async) · line 607

Quoted 6 lines

run-command.c +0 -0

index e31c073..46ea07d 100644
@@ -607,7 +607,7 @@ void abort_async(struct async *async)
  #ifdef NO_PTHREADS

This context-line doesn't match 3/4... Did you send out the wrong version of that patch?

Quoted 16 lines · Jeff King

We just need to cancel the thread, instead of sending it a signal as we do for fork()'d async sections.

Signed-off-by Jeff King <[email protected]>

---
This could also just be part of the merge resolution, but I thought it would be easier to see what is going on if I put it here.

 run-command.c |    2 +-

 1 files changed, 1 insertions(+), 1 deletions(-)

run-command.c +0 -0

index e31c073..46ea07d 100644

run-command.c void abort_async(struct async *async) · line 607

Quoted 1 line

run-command.c +0 -0

@@ -607,7 +607,7 @@ void abort_async(struct async *async)
  #ifdef NO_PTHREADS

This context-line doesn't match 3/4... Did you send out the wrong version of that patch?

run-command.c void abort_async(struct async *async) · line 610

Quoted 4 lines

run-command.c +1 -1

        kill(async->pid, 15);
  #else
-       /* no clue */
+       pthread_cancel(async->tid);

My worry about terminating a thread that's currently holding a mutex (implicitly through the CRT?) still applies though...

Quoted 8 lines · Erik Faye-Lund

Quoted 4 lines · Jeff King

       kill(async->pid, 15);
 #else
-       /* no clue */
+       pthread_cancel(async->tid);

My worry about terminating a thread that's currently holding a mutex (implicitly through the CRT?) still applies though...

OK, I've read up on thread-cancellation, and this code seems correct. pthread_cancel doesn't kill the thread right away, it just signals a cancellation-event, which is checked for at certain cancellation-points. A lot of the CRT functions are defined as cancellation points, so it'll be a matter for us Win32-guys to implement pthread_testcancel() and inject that into the function-wrappers of the CRT functions that are marked as cancellation-points.

Quoted 8 lines

OK, I've read up on thread-cancellation, and this code seems correct. pthread_cancel doesn't kill the thread right away, it just signals a cancellation-event, which is checked for at certain cancellation-points. A lot of the CRT functions are defined as cancellation points, so it'll be a matter for us Win32-guys to implement pthread_testcancel() and inject that into the function-wrappers of the CRT functions that are marked as cancellation-points.

That's not going to happen. We cannot implement pthread_cancel() on Windows because it would have to be able to interrupt blocking system calls. (TerminateThread() is a no-no, given all the caveats about leaking system resources that are mentioned in the manual.)

[OK, "cannot" is a hard word. It is possible in some way, I'm sure. But that would mean that we implement the equivalent of Cygwin or so...]

But if I understand correctly what Jeff wrote so far, then the pthreaded case happens to work - by chance or by design, we don't know (yet). Perhaps we can get away with

Quoted 10 lines · Erik Faye-Lund

Quoted 8 lines · Erik Faye-Lund

Quoted 4 lines · Jeff King

       kill(async->pid, 15);
 #else
-       /* no clue */
+       pthread_cancel(async->tid);

My worry about terminating a thread that's currently holding a mutex (implicitly through the CRT?) still applies though...

Quoted 8 lines

OK, I've read up on thread-cancellation, and this code seems correct. pthread_cancel doesn't kill the thread right away, it just signals a cancellation-event, which is checked for at certain cancellation-points. A lot of the CRT functions are defined as cancellation points, so it'll be a matter for us Win32-guys to implement pthread_testcancel() and inject that into the function-wrappers of the CRT functions that are marked as cancellation-points.

That's not going to happen. We cannot implement pthread_cancel() on Windows because it would have to be able to interrupt blocking system calls. (TerminateThread() is a no-no, given all the caveats about leaking system resources that are mentioned in the manual.)

Quoted 10 lines · Johannes Sixt

Quoted 8 lines · Erik Faye-Lund

OK, I've read up on thread-cancellation, and this code seems correct. pthread_cancel doesn't kill the thread right away, it just signals a cancellation-event, which is checked for at certain cancellation-points. A lot of the CRT functions are defined as cancellation points, so it'll be a matter for us Win32-guys to implement pthread_testcancel() and inject that into the function-wrappers of the CRT functions that are marked as cancellation-points.

Quoted 4 lines

That's not going to happen. We cannot implement pthread_cancel() on Windows because it would have to be able to interrupt blocking system calls. (TerminateThread() is a no-no, given all the caveats about leaking system resources that are mentioned in the manual.)

Did you read my suggestion?

I was talking about implementing
cancellation-points, just like on other platforms. This should not lead to TerminateThread, but instead a conditional ExitThread from the thread in question.

Something like this (I've only added a cancellation-point at close, just to illustrate what I mean):

Quoted 11 lines · Erik Faye-Lund

Quoted 10 lines · Johannes Sixt

Quoted 8 lines · Erik Faye-Lund

OK, I've read up on thread-cancellation, and this code seems correct. pthread_cancel doesn't kill the thread right away, it just signals a cancellation-event, which is checked for at certain cancellation-points. A lot of the CRT functions are defined as cancellation points, so it'll be a matter for us Win32-guys to implement pthread_testcancel() and inject that into the function-wrappers of the CRT functions that are marked as cancellation-points.

Quoted 6 lines

Quoted 4 lines

That's not going to happen. We cannot implement pthread_cancel() on Windows because it would have to be able to interrupt blocking system calls. (TerminateThread() is a no-no, given all the caveats about leaking system resources that are mentioned in the manual.)

Did you read my suggestion?

Yes, I did.

Quoted 5 lines

I was talking about implementing
cancellation-points, just like on other platforms. This should not lead to TerminateThread, but instead a conditional ExitThread from the thread in question.

Quoted 2 lines

Something like this (I've only added a cancellation-point at close, just to illustrate what I mean):

But this does not help the case at hand in any way. How would you interrupt a thread that is blocked in ReadFile()? The point of pthread_cancel() is that it interrupts blocked system calls

Quoted 3 lines

But this does not help the case at hand in any way. How would you interrupt a thread that is blocked in ReadFile()? The point of pthread_cancel() is that it interrupts blocked system calls

There is no mention of such a guarantee in POSIX (section 2.9.5 Thread Cancellation), so relying on that is undefined behavior.

Quoted 4 lines · Erik Faye-Lund

Quoted 3 lines

But this does not help the case at hand in any way. How would you interrupt a thread that is blocked in ReadFile()? The point of pthread_cancel() is that it interrupts blocked system calls

Quoted 2 lines

There is no mention of such a guarantee in POSIX (section 2.9.5 Thread Cancellation), so relying on that is undefined behavior.

Eh? My pthreads(7) says that read() is required to be a cancellation point acrroding to POSIX. I didn't dig up the actual reference in the standard, though.

Quoted 9 lines · Jeff King

Quoted 6 lines · Erik Faye-Lund

Quoted 3 lines

But this does not help the case at hand in any way. How would you interrupt a thread that is blocked in ReadFile()? The point of pthread_cancel() is that it interrupts blocked system calls

There is no mention of such a guarantee in POSIX (section 2.9.5 Thread Cancellation), so relying on that is undefined behavior.

Quoted 3 lines

Eh? My pthreads(7) says that read() is required to be a cancellation point acrroding to POSIX. I didn't dig up the actual reference in the standard, though.

I don't understand where the implementor would get that from after reading through it, but if there's something I've missed we can fix it by replacing my pthread_cancel with this, no?

static inline int pthread_cancel(pthread_t thread)

{
	SetEvent(thread.cancel_event);
	CancelSynchronousIo(thread.handle);
}

Quoted 5 lines

static inline int pthread_cancel(pthread_t thread)

{
	SetEvent(thread.cancel_event);
	CancelSynchronousIo(thread.handle);
}

There are a ton of cancellation points, not just I/O (e.g., sleep). However, interrupting a read would probably be sufficient for git's purposes.

Quoted 14 lines · Erik Faye-Lund

Quoted 12 lines · Jeff King

Quoted 6 lines · Erik Faye-Lund

Quoted 3 lines

But this does not help the case at hand in any way. How would you interrupt a thread that is blocked in ReadFile()? The point of pthread_cancel() is that it interrupts blocked system calls

There is no mention of such a guarantee in POSIX (section 2.9.5 Thread Cancellation), so relying on that is undefined behavior.

Eh? My pthreads(7) says that read() is required to be a cancellation point acrroding to POSIX. I didn't dig up the actual reference in the standard, though.

Quoted 3 lines

I don't understand where the implementor would get that from after reading through it, but if there's something I've missed we can fix it by replacing my pthread_cancel with this, no?

Out of curiosity, which POSIX are you reading? My page references POSIX.1-2001, but technically pthreads were originally defined in POSIX.1c-1995.

Quoted 11 lines · Jeff King

Quoted 6 lines · Erik Faye-Lund

Quoted 3 lines

But this does not help the case at hand in any way. How would you interrupt a thread that is blocked in ReadFile()? The point of pthread_cancel() is that it interrupts blocked system calls

There is no mention of such a guarantee in POSIX (section 2.9.5 Thread Cancellation), so relying on that is undefined behavior.

Eh? My pthreads(7) says that read() is required to be a cancellation point acrroding to POSIX. I didn't dig up the actual reference in the standard, though.

I don't understand where the implementor would get that from after reading through it, but if there's something I've missed we can fix it by replacing my pthread_cancel with this, no?

static inline int pthread_cancel(pthread_t thread)

{
	SetEvent(thread.cancel_event);
	CancelSynchronousIo(thread.handle);
}

Quoted 17 lines · Erik Faye-Lund

Quoted 11 lines · Jeff King

Quoted 6 lines · Erik Faye-Lund

Quoted 3 lines

But this does not help the case at hand in any way. How would you interrupt a thread that is blocked in ReadFile()? The point of pthread_cancel() is that it interrupts blocked system calls

There is no mention of such a guarantee in POSIX (section 2.9.5 Thread Cancellation), so relying on that is undefined behavior.

Eh? My pthreads(7) says that read() is required to be a cancellation point acrroding to POSIX. I didn't dig up the actual reference in the standard, though.

I don't understand where the implementor would get that from after reading through it, but if there's something I've missed we can fix it by replacing my pthread_cancel with this, no?

Quoted 5 lines

static inline int pthread_cancel(pthread_t thread) {
      SetEvent(thread.cancel_event); CancelSynchronousIo(thread.handle); }

Do I deserve this? There *is* a function that does what we need. I get what I deserve when I don't study all 100,000 functions in the manual...

Wait a minute! I *did* study 100,000 functions. This is one of the 25,000 functions that are new in Vista... Those I haven't studied, yet.

Quoted 3 lines · Johannes Sixt

But this does not help the case at hand in any way. How would you interrupt a thread that is blocked in ReadFile()? The point of pthread_cancel() is that it interrupts blocked system calls

There is no mention of such a guarantee in POSIX (section 2.9.5 Thread Cancellation), so relying on that is undefined behavior.

Quoted 7 lines · Erik Faye-Lund

Quoted 3 lines · Johannes Sixt

But this does not help the case at hand in any way. How would you interrupt a thread that is blocked in ReadFile()? The point of pthread_cancel() is that it interrupts blocked system calls

There is no mention of such a guarantee in POSIX (section 2.9.5 Thread Cancellation), so relying on that is undefined behavior.

In the paragraph before the bulleted list at the end of "Cancellation Points":

"...If a thread has cancelability enabled and a cancellation request is made with the thread as a target while the thread is suspended at a cancellation point, the thread shall be awakened and the cancellation request shall be acted upon..."

Quoted 9 lines · Johannes Sixt

Quoted 7 lines · Erik Faye-Lund

Quoted 3 lines · Johannes Sixt

But this does not help the case at hand in any way. How would you interrupt a thread that is blocked in ReadFile()? The point of pthread_cancel() is that it interrupts blocked system calls

There is no mention of such a guarantee in POSIX (section 2.9.5 Thread Cancellation), so relying on that is undefined behavior.

Quoted 7 lines

In the paragraph before the bulleted list at the end of "Cancellation Points":

"...If a thread has cancelability enabled and a cancellation request is made with the thread as a target while the thread is suspended at a cancellation point, the thread shall be awakened and the cancellation request shall be acted upon..."

A blocking thread and a suspended are two different matters. A suspended thread is a thread that has been explicitly suspended by wait, waitpid, sleep, pause etc. These functions explicitly say that they suspend the thread ("shall suspend the calling thread until"), while read etc does not ("shall block the calling thread until").

Similarly, making a blocking read/write fail (or terminate mid-way) is not the same thing as awakening the thread.

I see how some people can read something like this into this section, but I think it's pretty clear - this is not what it's talking about. In fact, the more I read the relevant texts, the more convinced I get that implementations that does terminate read/write strictly speaking is in violation of the standard.

Quoted 24 lines · Erik Faye-Lund

Quoted 9 lines · Johannes Sixt

Quoted 7 lines · Erik Faye-Lund

Quoted 3 lines · Johannes Sixt

But this does not help the case at hand in any way. How would you interrupt a thread that is blocked in ReadFile()? The point of pthread_cancel() is that it interrupts blocked system calls

There is no mention of such a guarantee in POSIX (section 2.9.5 Thread Cancellation), so relying on that is undefined behavior.

Quoted 7 lines

In the paragraph before the bulleted list at the end of "Cancellation Points":

"...If a thread has cancelability enabled and a cancellation request is made with the thread as a target while the thread is suspended at a cancellation point, the thread shall be awakened and the cancellation request shall be acted upon..."

A blocking thread and a suspended are two different matters. A suspended thread is a thread that has been explicitly suspended by wait, waitpid, sleep, pause etc. These functions explicitly say that they suspend the thread ("shall suspend the calling thread until"), while read etc does not ("shall block the calling thread until").

Quoted 8 lines

Similarly, making a blocking read/write fail (or terminate mid-way) is not the same thing as awakening the thread.

I see how some people can read something like this into this section, but I think it's pretty clear - this is not what it's talking about. In fact, the more I read the relevant texts, the more convinced I get that implementations that does terminate read/write strictly speaking is in violation of the standard.

What about the text right before the bit that Johannes quoted:

 The side effects of acting upon a cancellation request while suspended during a call of a function are the same as the side effects that may be seen in a single-threaded program when a call to a function is interrupted by a signal and the given function returns [EINTR]. Any such side effects occur before any cancellation cleanup handlers are called.

I agree it would be nicer if it explicitly said "when you are in a function which is a cancellation point, pending cancellation requests which are delivered are acuted upon immediately".

But it is implied to me by the surrounding text, and it's the only sensible behavior IMHO.

Plus it seems to be what at least glibc pthreads
does on Linux, so I'm going to assume that people smarter than me thought about it and came up with the same interpretation.

My test program was:

-- >8 --
#include <pthread.h>
#include <unistd.h>
#include <stdio.h>

void *child(void *data)
{
 char buf[32];
 int r;

 fprintf(stderr, "child reading from stdin...\n"); r = read(0, buf, sizeof(buf)); fprintf(stderr, "child read returned %d\n", r); return NULL;
}

int main(void)
{
 pthread_t t;
 void *r;

 pthread_create(&t, NULL, child, NULL);
 sleep(3);
 pthread_cancel(t);

 pthread_join(t, &r);
 if (r == PTHREAD_CANCELED)
   fprintf(stderr, "thread was canceled\n");
 else
   fprintf(stderr, "thread returned %p\n", r);

 return 0;
}
-- >8 --

If you input something before 3 seconds is up, the thread prints its message and returns NULL. But if you let it go, the cancel interrupts the read().

Quoted 20 lines · Johannes Sixt

Quoted 10 lines · Erik Faye-Lund

Quoted 8 lines · Erik Faye-Lund

Quoted 4 lines · Jeff King

       kill(async->pid, 15);
 #else
-       /* no clue */
+       pthread_cancel(async->tid);

My worry about terminating a thread that's currently holding a mutex (implicitly through the CRT?) still applies though...

Quoted 8 lines

OK, I've read up on thread-cancellation, and this code seems correct. pthread_cancel doesn't kill the thread right away, it just signals a cancellation-event, which is checked for at certain cancellation-points. A lot of the CRT functions are defined as cancellation points, so it'll be a matter for us Win32-guys to implement pthread_testcancel() and inject that into the function-wrappers of the CRT functions that are marked as cancellation-points.

Quoted 5 lines

That's not going to happen. We cannot implement pthread_cancel() on Windows because it would have to be able to interrupt blocking system calls. (TerminateThread() is a no-no, given all the caveats about leaking system resources that are mentioned in the manual.)

Did you read my suggestion? I was talking about implementing cancellation-points, just like on other platforms. This should not lead to TerminateThread, but instead a conditional ExitThread from the thread in question.

Something like this (I've only added a cancellation-point at close, just to illustrate what I mean):

compat/mingw.c +8 -0

index 878b1de..253be14 100644
@@ -302,6 +302,14 @@ int mingw_open (const char *filename, int oflags, ...)
        return fd;
  }
 
+#undef close
+int mingw_close(int fd)
+{
+       int ret = close(fd);
+       pthread_testcancel();
+       return ret;
+}
+
  #undef write
  ssize_t mingw_write(int fd, const void *buf, size_t count)
  {

compat/mingw.h +3 -0

index 62eccd3..3e904c8 100644
@@ -175,6 +175,9 @@ int mingw_rmdir(const char *path);
  int mingw_open (const char *filename, int oflags, ...);
  #define open mingw_open
 
+int mingw_close(int fd);
+#define close mingw_close
+
  ssize_t mingw_write(int fd, const void *buf, size_t count);
  #define write mingw_write
 

compat/win32/pthread.c +4 -0

index 010e875..47d620b 100644
@@ -18,6 +18,7 @@ static unsigned __stdcall win32_start_routine(void *arg)
        pthread_t *thread = arg;
        thread->tid = GetCurrentThreadId();
        thread->arg = thread->start_routine(thread->arg);
+       CloseHandle(thread->cancel_event);
        return 0;
  }
 

@@ -26,6 +27,9 @@ int pthread_create(pthread_t *thread, const void *unused,
  {
        thread->arg = arg;
        thread->start_routine = start_routine;
+       thread->cancel_event = CreateEvent(NULL, FALSE, FALSE, NULL);
+       if (!thread->cancel_event)
+               die("failed to create event");
        thread->handle = (HANDLE)
                _beginthreadex(NULL, 0, win32_start_routine, thread, 0, NULL);
 

compat/win32/pthread.h +1 -0

index 2e20548..7fce39d 100644
@@ -56,6 +56,7 @@ extern int pthread_cond_broadcast(pthread_cond_t *cond);
  */
  typedef struct {
        HANDLE handle;
+       HANDLE cancel_event;
        void *(*start_routine)(void*);
        void *arg;
        DWORD tid;

  1. do nothing. I'm not 100% sure why, but the bug does not manifest itself with pthreads. I don't know how it behaves on win32.

Quoted 2 lines · Jeff King

  1. do nothing. I'm not 100% sure why, but the bug does not manifest itself with pthreads. I don't know how it behaves on win32.

The reason might be that with threads we carefully close file descriptors, so that the other end sees EOF or broken pipe and terminates, while with a forked sideband demux one of the channels remains open in the forked process?