Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

gearbox serve: LazyWriter problem #26

Open
patrickdepinguin opened this issue Jun 2, 2020 · 4 comments
Open

gearbox serve: LazyWriter problem #26

patrickdepinguin opened this issue Jun 2, 2020 · 4 comments

Comments

@patrickdepinguin
Copy link

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

@amol-
Copy link
Member

amol- commented Jun 2, 2020

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.

@kiilerix
Copy link

kiilerix commented Jun 3, 2020

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)

@amol-
Copy link
Member

amol- commented Jun 3, 2020

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

@kiilerix
Copy link

kiilerix commented Jun 3, 2020

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

( FWIW, https://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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants