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

util: Log early messages #16112

Open
wants to merge 3 commits into
base: master
from

Conversation

Projects
None yet
8 participants
@MarcoFalke
Copy link
Member

commented May 28, 2019

Early log messages are dropped on the floor and they'd never make it to the console or debug log. This can be tested by running the test included in this pull request without re-compiling the bitcoind.

Fix that by buffering early messages and flushing them as soon as all logging options have been initialized and logging has been started.

This pull request is identical to "Log early messages with -printtoconsole" (#13088) by ajtowns, with the following changes:

  • Rebased
  • Added docstrings for m_buffering and StartLogging
  • Switch CCriticalSection (aka RecursiveMutex) to just Mutex in the last commit
  • Added tests

Fixes #16098
Fixes #13157
Closes #13088

@MarcoFalke MarcoFalke removed the Tests label May 28, 2019

@MarcoFalke MarcoFalke force-pushed the MarcoFalke:1905-bufferLog branch 2 times, most recently from fab38de to fa9bc80 May 28, 2019

@promag
Copy link
Member

left a comment

Tested ACK fa9bc80, I don't see a problem regarding the new circular dependency.

Show resolved Hide resolved src/init.cpp

@MarcoFalke MarcoFalke force-pushed the MarcoFalke:1905-bufferLog branch 4 times, most recently from fa10f8b to faaba72 May 28, 2019

@MarcoFalke

This comment has been minimized.

Copy link
Member Author

commented May 28, 2019

I don't see a problem regarding the new circular dependency.

It would lead to issues in debug builds when a potential deadlock was detected, so I've reverted that for now.

@DrahtBot

This comment has been minimized.

Copy link
Contributor

commented May 28, 2019

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #16224 (gui: Bilingual GUI error messages by hebasto)
  • #16115 (On bitcoind startup, write config args to debug.log by LarryRuane)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

@hebasto
Copy link
Member

left a comment

Concept ACK.

Show resolved Hide resolved src/init.cpp
Show resolved Hide resolved src/logging.cpp
Show resolved Hide resolved src/test/setup_common.cpp Outdated
Show resolved Hide resolved src/init.cpp Outdated

ajtowns and others added some commits May 3, 2018

Replace OpenDebugLog() with StartLogging()
StartLogging() is used to mark the start of logging generically, whether
using -printtoconsole or -debuglogfile.
Log early messages with -printtoconsole
This ensures log messages prior to StartLogging() are replayed to
the console as well as to the debug log file.

@MarcoFalke MarcoFalke force-pushed the MarcoFalke:1905-bufferLog branch from faaba72 to fa6bb8a May 28, 2019

@sipa

This comment has been minimized.

Copy link
Member

commented May 28, 2019

Concept ACK

@MarcoFalke MarcoFalke force-pushed the MarcoFalke:1905-bufferLog branch from bb96bd0 to faa2a47 May 29, 2019

@practicalswift

This comment has been minimized.

Copy link
Member

commented May 29, 2019

Is there any reason to buffer the writing to stdout (m_print_to_console)?

@ajtowns

This comment has been minimized.

Copy link
Contributor

commented May 31, 2019

Is there any reason to buffer the writing to stdout (m_print_to_console)?

If you have bitcoind -noconnect=0 but have printtoconsole=0 in bitcoind.conf, then the "confusing double-negative" error will be queued when parsing the command line, but m_print_to_console can't be set to false until later when the config file is actually read.

@ajtowns

This comment has been minimized.

Copy link
Contributor

commented May 31, 2019

I've done #16127 to see what it would take to keep the thread safety annotations without the dependency loop (and hopefully also without the bugs), and what it'd look like to do the same thing elsewhere. Seems better to do that later (or not at all) than to add it to this PR to me...

@ajtowns

This comment has been minimized.

Copy link
Contributor

commented May 31, 2019

utACK faa2a47

@MarcoFalke MarcoFalke changed the title log: Log early messages util: Log early messages Jun 11, 2019

@kristapsk
Copy link
Contributor

left a comment

ACK faa2a47 (ran added functional test before / after recompiling, didn't do additional testing)

@practicalswift

This comment has been minimized.

Copy link
Member

commented Jun 12, 2019

@MarcoFalke, why buffer also the writing to stdout? Don't we want to print to stdout as soon as possible to be on the safe side? I think it makes sense to keep the "print to console" code path as simple/dumb as possible. Especially given the amount of trouble we've had with getting "just-after-process-launch-logging" working robustly :-)

Is there any reason to buffer the writing to stdout (m_print_to_console)?

@MarcoFalke

This comment has been minimized.

Copy link
Member Author

commented Jun 17, 2019

@MarcoFalke

This comment has been minimized.

Copy link
Member Author

commented Jun 17, 2019

Unless there are objections, this will be merged tomorrow

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.