Re: [msysGit] [PATCH/RFC 1/5] mingw: make fgetc raise SIGINT if apropriate
Hi kusma, On Sat, 1 Dec 2012, Erik Faye-Lund wrote: On Fri, Nov 30, 2012 at 6:58 PM, Johannes Schindelin johannes.schinde...@gmx.de wrote: Hi, On Tue, 13 Nov 2012, Erik Faye-Lund wrote: Set a control-handler to prevent the process from terminating, and simulate SIGINT so it can be handled by a signal-handler as usual. One thing you might want to mention is that the fgetc() handling is not thread-safe, and intentionally so: if two threads read from the same console, we are in trouble anyway. I'm not entirely sure if I know what you mean. Do you suggest that two threads can race for setting the console ctrl-handler? That was my idea, yes. I don't think that's the case; SetConsoleCtrlHandler(x, TRUE) adds a console handler to the handler-chain, and SetConsoleCtrlHandler(x, FALSE) removes it. If two threads add handlers, it is my understanding that one of them will be run, only to report no, no more ctrl-handling needed. Since both handlers block further ctrl-handling, I don't think there's a problem. My idea was that the SetConsoleCtrlHandler(x, FALSE) could remove the handler prematurely iff another thread wanted to install the very same handler (but it was already installed by the first thread). But as I said: for this to happen, *two* threads need to want to access the console for reading. In that case we'd be in bigger trouble than thread unsafety... We cannot read two passwords from the same console at the same time. Ciao, Dscho -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [msysGit] [PATCH/RFC 1/5] mingw: make fgetc raise SIGINT if apropriate
On Tue, Dec 4, 2012 at 6:12 PM, Johannes Schindelin
johannes.schinde...@gmx.de wrote:
Hi kusma,
On Sat, 1 Dec 2012, Erik Faye-Lund wrote:
On Fri, Nov 30, 2012 at 6:58 PM, Johannes Schindelin
johannes.schinde...@gmx.de wrote:
Hi,
On Tue, 13 Nov 2012, Erik Faye-Lund wrote:
Set a control-handler to prevent the process from terminating, and
simulate SIGINT so it can be handled by a signal-handler as usual.
One thing you might want to mention is that the fgetc() handling is not
thread-safe, and intentionally so: if two threads read from the same
console, we are in trouble anyway.
I'm not entirely sure if I know what you mean. Do you suggest that two
threads can race for setting the console ctrl-handler?
That was my idea, yes.
I don't think that's the case; SetConsoleCtrlHandler(x, TRUE) adds a
console handler to the handler-chain, and SetConsoleCtrlHandler(x,
FALSE) removes it. If two threads add handlers, it is my understanding
that one of them will be run, only to report no, no more ctrl-handling
needed. Since both handlers block further ctrl-handling, I don't think
there's a problem.
My idea was that the SetConsoleCtrlHandler(x, FALSE) could remove the
handler prematurely iff another thread wanted to install the very same
handler (but it was already installed by the first thread).
Yes, but that's not how SetConsoleCtrlHandler works; if a routine x
gets added twice, it needs to be removed twice as well to not get
called. This little C-program demonstrates that:
---8---
#include windows.h
#include stdio.h
BOOL WINAPI HandlerRoutine1(DWORD dwCtrlType)
{
printf(Hello from handler1!\n);
return TRUE;
}
BOOL WINAPI HandlerRoutine2(DWORD dwCtrlType)
{
printf(Hello from handler2!\n);
return FALSE;
}
int main()
{
SetConsoleCtrlHandler(HandlerRoutine1, TRUE);
SetConsoleCtrlHandler(HandlerRoutine2, TRUE);
SetConsoleCtrlHandler(HandlerRoutine2, TRUE);
SetConsoleCtrlHandler(HandlerRoutine2, FALSE);
GenerateConsoleCtrlEvent(CTRL_C_EVENT, 0);
SetConsoleCtrlHandler(HandlerRoutine2, FALSE);
SetConsoleCtrlHandler(HandlerRoutine1, FALSE);
}
---8---
This program outputs:
Hello from handler2!
Hello from handler1!
So since that other thread would add the console ctrl handler before
it removed it again, this should still be thread-safe as far as I can
tell.
But as I said: for this to happen, *two* threads need to want to access
the console for reading. In that case we'd be in bigger trouble than
thread unsafety... We cannot read two passwords from the same console at
the same time.
I agree with that, but I'm saying that *even if* we didn't have this
limitation, the code shouldn't be problematic.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [msysGit] [PATCH/RFC 1/5] mingw: make fgetc raise SIGINT if apropriate
On Fri, Nov 30, 2012 at 7:11 PM, Jeff King p...@peff.net wrote: On Fri, Nov 30, 2012 at 06:58:11PM +0100, Johannes Schindelin wrote: Hi, On Tue, 13 Nov 2012, Erik Faye-Lund wrote: Set a control-handler to prevent the process from terminating, and simulate SIGINT so it can be handled by a signal-handler as usual. One thing you might want to mention is that the fgetc() handling is not thread-safe, and intentionally so: if two threads read from the same console, we are in trouble anyway. That makes sense to me, but I'm confused why it is part of mingw_fgetc, which could in theory read from arbitrary streams, no? It it is not necessarily a console operation at all. I feel like I'm probably missing something subtle here... I did add an early out for the non-console cases. Is this what you're missing, perhaps? -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [msysGit] [PATCH/RFC 1/5] mingw: make fgetc raise SIGINT if apropriate
On Fri, Nov 30, 2012 at 6:58 PM, Johannes Schindelin johannes.schinde...@gmx.de wrote: Hi, On Tue, 13 Nov 2012, Erik Faye-Lund wrote: Set a control-handler to prevent the process from terminating, and simulate SIGINT so it can be handled by a signal-handler as usual. One thing you might want to mention is that the fgetc() handling is not thread-safe, and intentionally so: if two threads read from the same console, we are in trouble anyway. I'm not entirely sure if I know what you mean. Do you suggest that two threads can race for setting the console ctrl-handler? I don't think that's the case; SetConsoleCtrlHandler(x, TRUE) adds a console handler to the handler-chain, and SetConsoleCtrlHandler(x, FALSE) removes it. If two threads add handlers, it is my understanding that one of them will be run, only to report no, no more ctrl-handling needed. Since both handlers block further ctrl-handling, I don't think there's a problem. Do you care to clarify what your thread-safety complaint is? BTW I like the new mingw_raise() very much! Thanks! I originally implemented it for a different reason, but that patch didn't turn out to be useful, so it's nice to finally put it to use ;) -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [msysGit] [PATCH/RFC 1/5] mingw: make fgetc raise SIGINT if apropriate
On Sat, Dec 01, 2012 at 01:31:23PM +0100, Erik Faye-Lund wrote: One thing you might want to mention is that the fgetc() handling is not thread-safe, and intentionally so: if two threads read from the same console, we are in trouble anyway. That makes sense to me, but I'm confused why it is part of mingw_fgetc, which could in theory read from arbitrary streams, no? It it is not necessarily a console operation at all. I feel like I'm probably missing something subtle here... I did add an early out for the non-console cases. Is this what you're missing, perhaps? Oops, yes. That is exactly what I was missing. :) Sorry for the noise. -Peff -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [msysGit] [PATCH/RFC 1/5] mingw: make fgetc raise SIGINT if apropriate
Hi, On Tue, 13 Nov 2012, Erik Faye-Lund wrote: Set a control-handler to prevent the process from terminating, and simulate SIGINT so it can be handled by a signal-handler as usual. One thing you might want to mention is that the fgetc() handling is not thread-safe, and intentionally so: if two threads read from the same console, we are in trouble anyway. BTW I like the new mingw_raise() very much! Ciao, Dscho -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [msysGit] [PATCH/RFC 1/5] mingw: make fgetc raise SIGINT if apropriate
On Fri, Nov 30, 2012 at 06:58:11PM +0100, Johannes Schindelin wrote: Hi, On Tue, 13 Nov 2012, Erik Faye-Lund wrote: Set a control-handler to prevent the process from terminating, and simulate SIGINT so it can be handled by a signal-handler as usual. One thing you might want to mention is that the fgetc() handling is not thread-safe, and intentionally so: if two threads read from the same console, we are in trouble anyway. That makes sense to me, but I'm confused why it is part of mingw_fgetc, which could in theory read from arbitrary streams, no? It it is not necessarily a console operation at all. I feel like I'm probably missing something subtle here... -Peff -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
