On 07/09/2014 11:17 AM, Frank Ch. Eigler wrote:
brolley wrote:
Regarding commit 9021460e, which nathans already reviewed(?) / merged:
Implement overall timeout for service discovery.
-t=N.N, --timeout=N.N for pmfind. N.N is the timeout period
in seconds.
I have one concern: the new timeout parameter passed to the
pmDiscover* function doesn't actually work! What i mean is that it is
not sufficient to perform the timeout functionality by itself: it only
sets up a posix timer to send a SIGALARM sometime. It does not
consume that signal, but requires the client to register a SIGALRM
handler, and let it flip the options->interrupt flag. This embryonic
a service the pmfind app could have done for itself, by just calling
alarm(2)/setitimer(2).
I had a clever argument all prepared about how the client app was free
to respond to the timeout signal (SIGALRM) in any manner that it wishes
and that actually terminating the discovery was only one possible
reaction. However, the more I try to convince myself, the lamer it
sounds. Clearly a timeout is intended to be just that and, if needed at
all, the solution should be encapsulated within the libpcp discovery code.
A better approach would be to avoid presuming anything about the
signal handling state of the enveloping application (and thus not use
SIGALRM-generating facilities). For example, libpcp could spawn an
internal thread that just sleeps awhile, then sets the currently
operating context/interrupt flag. (The code would have to be careful
about races between that thread's activities vs. lifecycle, but it
doesn't sound too bad.)
KISS definitely applies. Either this is so simple that it can be left to
the client, as you mentioned, or something simple like the above. I see
that presuming anything about the app's signal handling state is
probably a bad idea.
Dave
|