Обсуждение: Win32 signals code, take two
Here's an updated version of the proposed win32 signals code, with the following main changes: * Now uses names and defines already in pqsignal.h when available - this is where it will end up after all. Only changes names. * The signal listener threads now creates separate threads to read/write on the pipe, to make sure we don't hang anywhere there. I put the ReadFile() in the separate thread as well (to be sure). It does not pose any different synchronisation issues than we had before. * The signal queue is now kept as a bitmask. * Signals queued while blocked are now delivered once unblocked * Default signals are now handled. They're all set to SIGIGN, because I haven't figured out what to put in there :-) * Signals can now be delivered at two locations: From a User APC queued by the signal handling thread, or when calling sigsetmask() and unblocking a signal that has been queued. * PG_POLL_SIGNALS() now does WaitForSingleObjectEx() on an object that actually exists, so no error is given. Those are the majors :-) Claudio, in your other working on the backend, have you by any chance identified good places to do: 1) pg_signal_initialize()? 2) PG_POLL_SIGNALS()? Also, another question on the interface: Right now throughout the backend, pqsignal() is used to modify signals, and not signal() directly. But sigsetmask(), kill() etc are called directly. Should we go with that, and as now just define our own, or should we change to pgsigsetmask(), pgkill() etc, and then change taht all over the code? Which is the preferred way? Apart from those, comments in general? I hope I managed to include all the things discussed since last time :-) //Magnus
Вложения
Hi Magnus, I've only had time for a very superficial look, but looks good... > * The signal listener threads now creates separate threads to read/write > on the pipe, to make sure we don't hang anywhere there. I put the > ReadFile() in the separate thread as well (to be sure). It > does not pose any different synchronisation issues than we had before. Hmm, I'd be happier just seeing the main thread issue these calls, but no firm argument as to why :-) > Claudio, in your other working on the backend, have you by any chance > identified good places to do: > 1) pg_signal_initialize()? I imagine we'll want to drop this main.c, right by the WSAStartup code. > 2) PG_POLL_SIGNALS()? Haven't had a chance to really try [sigh]. I promise to have a closer look, at this and at your code, over the next couple days. Talk soon, Claudio --- Certain disclaimers and policies apply to all email sent from Memetrics. For the full text of these disclaimers and policies see <a href="http://www.memetrics.com/emailpolicy.html">http://www.memetrics.com/em ailpolicy.html</a>
Magnus Hagander wrote: > Here's an updated version of the proposed win32 signals code, with the > following main changes: One small possible revision to consider: As I read the code, all manipulation to pg_signal_queue is inside a CriticalSection, and it is only set (> 0) when there are pending signals. ISTM that pg_queue_signal can abort without calling QueueUserAPC pg_signal_queue is already set. This will keep the dispatch function from getting called extra times. Paranoia statement pg_signal_queue = 0; could possibly be added at the end of the dispatch. Everything else looks good to me. Looking at the code, pg_signal_queue is better and simpler than the event based method I was suggesting, IMO. Merlin
> Magnus Hagander wrote: > > Here's an updated version of the proposed win32 signals > code, with the > > following main changes: > > One small possible revision to consider: As I read the code, > all manipulation to pg_signal_queue is inside a > CriticalSection, and it is only set (> 0) when there are > pending signals. > > ISTM that pg_queue_signal can abort without calling > QueueUserAPC pg_signal_queue is already set. This will keep > the dispatch function from getting called extra times. > Paranoia statement > > pg_signal_queue = 0; > > could possibly be added at the end of the dispatch. No, I don't think it can't. Consider delayed signals. When leaving the dispatch function, pg_signal_queue may very well be != 0. Only (pg_signal_queue & ~pg_signal_mask) should be zero. Also, I think it's best if the mask is checked upon signal *delivery*, not queueing. The signal could be blocked when delivered and non-blocked on delivery. If we never queue a APC in this case, the signal will be lost. But we *could* do the check in pg_signal_queue, but then against (pg_signal_queue &~ pg_signal_mask). //Magnus
Magnus Hagander wrote: > > Magnus Hagander wrote: > > > Here's an updated version of the proposed win32 signals > > code, with the > > > following main changes: > > > > One small possible revision to consider: As I read the code, > > all manipulation to pg_signal_queue is inside a > > CriticalSection, and it is only set (> 0) when there are > > pending signals. > > > > ISTM that pg_queue_signal can abort without calling > > QueueUserAPC pg_signal_queue is already set. This will keep > > the dispatch function from getting called extra times. > > Paranoia statement > > > > pg_signal_queue = 0; > > > > could possibly be added at the end of the dispatch. > > No, I don't think it can't. Consider delayed signals. When leaving the > dispatch function, pg_signal_queue may very well be != 0. Only > (pg_signal_queue & ~pg_signal_mask) should be zero. Yep...my bad. > Also, I think it's best if the mask is checked upon signal *delivery*, > not queueing. The signal could be blocked when delivered and non-blocked > on delivery. If we never queue a APC in this case, the signal will be > lost. But we *could* do the check in pg_signal_queue, but then against > (pg_signal_queue &~ pg_signal_mask). Right...that is safer. I still think there may be a more sophisticated check in there...if any executable signal is already pending, then you can count that you are either in a dispatch or that dispatch is already queued. Merlin
> I've only had time for a very superficial look, but looks good... One question: What about the same signal being raised multiple times whilst blocked? Currently, if a signal is raised twice whilst blocked, it will be delivered a single time when unblocked (it ought to be delivered twice). Cheers, Claudio --- Certain disclaimers and policies apply to all email sent from Memetrics. For the full text of these disclaimers and policies see <a href="http://www.memetrics.com/emailpolicy.html">http://www.memetrics.com/em ailpolicy.html</a>
> > I've only had time for a very superficial look, but looks good... > > One question: What about the same signal being raised > multiple times whilst blocked? Currently, if a signal is > raised twice whilst blocked, it will be delivered a single > time when unblocked (it ought to be delivered twice). They are only delivered once in this implementation. I was under the impression that's what we wanted? It is also the behaviour on my linux system. If I sigblock() HUP, then fire off 10 "kill -HUP <mypid>", and finally unblock it, the HUP handler is called exactly once. //magnus
Claudio Natoli wrote: >>I've only had time for a very superficial look, but looks good... >> >> > >One question: What about the same signal being raised multiple times whilst >blocked? Currently, if a signal is raised twice whilst blocked, it will be >delivered a single time when unblocked (it ought to be delivered twice). > > > Stevens APUE (s. 10.8) says: "What happens if a blocked signal is generated more than once before the process unblocks the signal? POSIX.1 allows the system to deliver the signal once or more than once. If the system delivers the signal more than once, we say the signals are queued. Most Unix systems, however, do not queue signals. Instead the Unix kernel just delivers the signal once." cheers andrew
> Stevens APUE (s. 10.8) says: > > "What happens if a blocked signal is generated more than once before > the process unblocks the signal? POSIX.1 allows the system to deliver > the signal once or more than once. If the system delivers the signal > more than once, we say the signals are queued. Most Unix systems, > however, do not queue signals. Instead the Unix kernel just > delivers the signal once." Thanks Andrew, I've only ever seen it the other way... guess it won't matter then. Cheers, Claudio --- Certain disclaimers and policies apply to all email sent from Memetrics. For the full text of these disclaimers and policies see <a href="http://www.memetrics.com/emailpolicy.html">http://www.memetrics.com/em ailpolicy.html</a>