First off, thanks Mike and Chris for taking a look at this. > > On 5/10/11 8:52 PM, Chris McDonough wrote: >> I've taken a look at the patch provided via >> https://github.com/Supervisor/supervisor/pull/10 . It ensures that, for >> rotated file logs at least, that if two processes log to the same >> filename, that the same stream is used for both processes. >> >> The issue that has been coming up when Whit sees file logs rotate (when >> a file size is rolled over, the handler for process A does rotation, but >> then process B's handler also does it too, which leads to weirdness). >> >> My question is: do we want to advertise support for two separate >> processes logging to the same file name in this way? > > If it's not going to cause other problems inside Supervisor, I don't see a > reason we should disallow it. > >> If not, I should probably work up a patch that logs a warning when the >> system detects that it's logging two process' files to the same filename >> and provide an alternate mechanism for doing the same thing (like >> syslog, or something else). > > I do think we should eventually implement syslog support regardless. This > has > come up a few times on the mailing list.
Maybe pluggable logging in general? > >> If so, the patch looks reasonable, but maybe a bit purpose-focused. >> I suspect we should instead apply it to the lower level FileHandler >> instead of to RotatingFileHandler. Maybe? it is specific to the act of rotation. I don't know of another case where you would need to track open file handles for logging, but I'm really don't know. > > I looked at an earlier version of this patch. It seemed that the tests > provided passed before the rest of the patch was applied. If I'm wrong or > that's no longer the case, please feel free to commit it or a variation of it. > I'll have to check. One of the tests was crafted specifically to illustrate the scattering inode problem and failed before patching (I wrote it first before the patch). That being said, I did quite a few contortions try to get the original github fork to be an actual official fork and something might have been lost in the process. > One other issue with the patch is that keys in the open_streams dict are > never > removed. The keys are the log filenames. If the configuration is reloaded > and the processes or log files are changed, it will "leak" keys in this dict. > I don't think this will be a problem for most Supervisor users because > configuration changes are infrequent. I have some customers using the > twiddler plugin in a special application where supervisord has a very long > uptime and the process configuration changes as often as every minute. I > believe the potential memory growth could be a problem for them. In general, it sounds like we should flush the filehandle dict on configuration change to prevent this leakage. A class attribute is sort of icky, but I didn't see a sensible place to store the filehandles as part of the state of the running server. -w >>> Whit Morriss CodeMonkey w...@surveymonkey.com _______________________________________________ Supervisor-users mailing list Supervisor-users@lists.supervisord.org http://lists.supervisord.org/mailman/listinfo/supervisor-users
