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

MainLogger: Do output to console on the logger thread itself #1983

Closed
wants to merge 1 commit into from

Conversation

dktapps
Copy link
Member

@dktapps dktapps commented Jan 30, 2018

This solves a range of issues (existing and potential) with logging, such as:

  • echo is like a disk operation (blocking I/O) which may produce main thread slowdown.
  • Logging to the console is not currently synchronized - I am not actually sure why console output doesn't become illegible gibberish when multiple threads use the logger at the same time. Maybe something to do with locking when accessing properties of the logger? (implicit synchronization). This change solves that by doing all output to console on a single thread.
  • In PowerShell, if the user selects some text in QuickEdit mode, the server will freeze next time it hits an echo call. This is because STDOUT write blocks until the user stops selecting text. This change resolves that problem.
  • This also opens a possible solution to Terminal::$formattingCodes has to be inherited for any thread which wants to use the MainLogger #1979 by handling escape code processing on the logger thread itself.

Changes

API changes

  • Added 2 new API methods - MainLogger->pushToOutputBuffer() and MainLogger->getOutputBufferContents()

Behavioural changes

  • Logging now echoes output on the logger thread itself instead of the calling thread, avoiding slowdowns and hangs due to console I/O.
  • Title ticking output is now done on the logger thread due to visible slowdown it causes on the main thread.

To be clear, we're usually talking fractions of a millisecond. This is primarily intended to resolve problems and not to provide performance improvements.

Backwards compatibility

This breaks tests as seen below - additionally, logger output can no longer be captured for a thread by using the PHP output buffer functions (ob_*()).

Tests

Has been tested and several bugs fixed (bug fixes have since been ported to master since they were not specific to this change). However, Travis is broken because TesterPlugin needs updating.

echo is nearly as bad as a disk operation (blocking I/O).

In PowerShell, if the user selects some text in QuickEdit mode, the server will freeze next time it hits an echo call. This is because STDOUT write blocks until the user stops selecting text. This change resolves that problem.
@dktapps dktapps added Category: Core Related to internal functionality Type: Fix Bug fix, typo fix, or any other fix Category: Cosmetic Related to formatting or code style labels Jan 30, 2018
@dktapps
Copy link
Member Author

dktapps commented Jan 30, 2018

The problem with this is that a block-up writing to disk or writing to console will affect each other, which is probably undesirable. It might be better to split up disk I/O and console I/O into two separate threads so that they can't affect each other.

@dktapps
Copy link
Member Author

dktapps commented May 4, 2018

This can be better done under a new planned logger API with isolated threaded logger attachments. As I mentioned above, disk and console I/O should not affect each other, but will if either one blocks.

@dktapps dktapps closed this May 4, 2018
@dktapps dktapps deleted the more-threaded-logging branch May 4, 2018 20:43
@dktapps dktapps added the Resolution: Declined PR has been declined by maintainers label May 4, 2018
@dktapps dktapps removed the Category: Cosmetic Related to formatting or code style label Nov 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Category: Core Related to internal functionality Resolution: Declined PR has been declined by maintainers Type: Fix Bug fix, typo fix, or any other fix
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant