gearbox icon indicating copy to clipboard operation
gearbox copied to clipboard

gearbox serve: LazyWriter problem

Open patrickdepinguin opened this issue 4 years ago • 4 comments

gearbox serve patches stdout/stderr to be an object of class LazyWriter, which only opens the file when something is written. Is this only to avoid an empty log file in case an application never actually writes anything?

In the Kallithea project, a problem was reported [1] which is actually not related to Kallithea itself but by the interaction between Mercurial code and gearbox Lazywriter. Mercurial wants to access the 'buffer' property of stdout/stderr, but Lazywriter does not implement it.

In order to work around it from Kallithea, we have to do something like [2] which is not very nice.

A possible alternative is that LazyWriter actually forwards any attribute access it doesn't know about to the real file object (but in this case it should actually open the file).

Another alternative is to just stop using LazyWriter. If the only goal is to not create an empty log file when the app happens to be silent, then I don't consider it as having much value. Many UNIX applications that accept a log file path as argument will create the file regardless of output.

Maybe other solutions exist too.

[1] https://bitbucket.org/conservancy/kallithea/issues/371 [2] https://kallithea-scm.org/repos/kallithea-incoming/changeset/ba656cdb88eae618beb3d6b83b2c14584b9233d8

patrickdepinguin avatar Jun 02 '20 14:06 patrickdepinguin

LazyWriter goes back to the time of PasteScript, so it's hard to tell why it was put there. But if I had to guess I'd think more in the direction of daemonization code, than to just avoid creating unecessary log files.

In this case I think that probably we could easily add the buffer property to LazyWriter as it's expected to mimic sys.stdout and it was probably missed when code was ported to Python3.

If you are willing to submit a PR that adds the buffer property to LazyWriter I'll gladly merge it, so that you can test if that solves the issue for Kallithea.

amol- avatar Jun 02 '20 20:06 amol-

LazyWriter is only used for explicit use of --log-file.

I don't know which daemonization cases could be relevant. I would expect the daemonization (like systemd) to capture stdout / stderr, and it would be very wrong to use --log-file for that use case. Also because it probably wouldn't work nicely with multiple instances writing to the same file.

And when using --log-file I would appreciate if it instantly created the file, instead of only doing it if something is logged. That would make the behaviour much more predictable.

I thus suggest just removing LazyWriter and use

stdout_log = open(self.app.options.log_file, 'a')

(barely worth creating a PR for this)

kiilerix avatar Jun 03 '20 12:06 kiilerix

By daemonization I actually meant the --daemon option of gearbox

amol- avatar Jun 03 '20 13:06 amol-

Ok. I don't see --daemon in gearbox -h or gearbox serve -h . (That might be a general problem - I don' know where.)

( FWIW, http://aspn.activestate.com/ASPN/Cookbook/Python/Recipe/278731 is also dead )

But AFAICS, there shouldn't be a problem with --daemon and --log-file opening the file immediately. Quite the opposite: it would be better to have a "so far empty logfile" than not having a log file at all.

kiilerix avatar Jun 03 '20 20:06 kiilerix