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

Add flush to the file write functions #58

Merged
merged 4 commits into from
Jun 16, 2024

Conversation

ilikecake
Copy link
Contributor

Sorry for the churn. Further testing of the logging file handlers showed that they needed a flush() after the write() command in order for the contents of the log file to be updated in a reasonable time.

Copy link
Contributor

@dhalbert dhalbert left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the CPython version of this library, StreamHandler provides a flush() operation, and so do its subclasses, such as FileHandler.

I think the user should be able to use flush() as needed to guarantee that a log entry has been written to a file, but it should be optional. There are use cases where you don't want to always do a flush(). This change will cause more wear on the flash chips.

@ilikecake
Copy link
Contributor Author

Added a flush() function to the StreamHandler. It should be inherited by the file handler classes below it. There does not seem to be a good way to actually call this function, however. Do we also need to impelment the shutdown() function?

https://docs.python.org/3/library/logging.html#logging.shutdown

@ilikecake
Copy link
Contributor Author

How about this? I added a flushHandlers() function to the logger. This is not in the default python implementation of the logger, but I can't see any other way to force a flush of the handlers without accessing the semi-private _handlers list from user code.

@dhalbert
Copy link
Contributor

dhalbert commented Jun 4, 2024

I am sorry! I have led you astray. It turns out the that in the CPython impl, StreamHandler flushes by default: https://github.com/python/cpython/blob/3bf7a5079c8d1845af98c145056e7ae1e102be43/Lib/logging/__init__.py#L1164.

If you didn't want this to happen, you can create your own handlers and have it implements a particular strategy. There is a superclass BufferingHandler, which has a shouldFlush() operation: https://github.com/python/cpython/blob/3bf7a5079c8d1845af98c145056e7ae1e102be43/Lib/logging/handlers.py#L1304

The CPython library has a .handlers public property which returns all the registered handlers. But apparently it's discouraged to use this to flush: https://stackoverflow.com/questions/13176173/python-how-to-flush-the-log-django.

We try not to add features to the CPython library API, so I'd be reluctant to add flushHandlers(). I'm thiking we should go back to your original auto-flush() in 337770e.

If desired, a separate library could be added that would define a non-flushing file handler, not based on StreamHandler. I am not suggesting it is worth doing that now.

@ilikecake
Copy link
Contributor Author

Sounds good, I have reverted it back to calling flush() after write().

Copy link
Contributor

@FoamyGuy FoamyGuy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me.

Thanks for working on this followup @ilikecake!

@FoamyGuy FoamyGuy merged commit c9c6450 into adafruit:main Jun 16, 2024
1 check passed
adafruit-adabot added a commit to adafruit/Adafruit_CircuitPython_Bundle that referenced this pull request Jun 17, 2024
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

Successfully merging this pull request may close these issues.

3 participants