This PR improves and fixes i2p logs (joint work with vasild).
LogPrint to LogPrintLevel so we can log according to the severity.Accept. Before this PR, when an interruption happens, it just logs “Error accepting:”, no reason is logged as it does for other situations. This PR changes it to log “Accept interrupted”.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For detailed information about the code coverage, see the test coverage report.
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | marcofleon, vasild, achow101 |
| Concept ACK | jonatack |
| Stale ACK | kevkevinpal, laanwj |
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
Reviewers, this pull request conflicts with the following ones:
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.
201@@ -202,7 +202,11 @@ bool Session::Accept(Connection& conn)
202 return true;
203 }
204
205- Log("Error accepting%s: %s", disconnect ? " (will close the session)" : "", errmsg);
206+ if (m_interrupt) {
207+ LogPrintLevel(BCLog::I2P, BCLog::Level::Debug, "Accept interrupted\n");
Rebased to re-run CI.
Better to ask someone in the IRC channel to re-run the CI, as that won’t invalidate ACKs 😄
re-ACK 469e4834bad295f6d9fbf8048a8db23aa758bac9
fc0a50e (#25203) and 43315a0 (#25203) from two years ago, in case you’d like to compare/check against them.
448@@ -451,7 +449,7 @@ void Session::CreateIfNotCreatedAlready()
449 m_session_id = session_id;
450 m_control_sock = std::move(sock);
451
452- Log("%s SAM session %s created, my address=%s",
453+ LogPrintLevel(BCLog::I2P, BCLog::Level::Debug, "%s SAM session %s created, my address=%s\n",
Commit 48dc2ff8e7819e810d2436219268f220b39c14f
in fc0a50e92aaaf3b049fcfcd0873dadc7150cfecf I made this one info (default) level here. It seems important, when the user turns i2p logging on, to see if the session was successfully created. I look for this logging when launching a node or restarting the I2P router.
-i2pacceptincoming=0) this would be printed for every I2P peer connect and disconnect even if I2P logging is switched off.
Note that for transient I2P sessions (when
-i2pacceptincoming=0) this would be printed for every I2P peer connect and disconnect even if I2P logging is switched off.
Good point. At the same time, I2P connections are intended to be long-running ones, as the tunnels are one-way and more time/resource intensive to launch than other networks. When i2pacceptincoming is off (which we discourage in doc/i2p.md), I’m unsure whether it’s best for this to remain verbose, or to set it as a less-frequent log level (debug) in that case.
479@@ -482,9 +480,9 @@ void Session::Disconnect()
480 {
481 if (m_control_sock) {
482 if (m_session_id.empty()) {
483- Log("Destroying incomplete SAM session");
484+ LogPrintLevel(BCLog::I2P, BCLog::Level::Debug, "Destroying incomplete SAM session\n");
485 } else {
486- Log("Destroying SAM session %s", m_session_id);
487+ LogPrintLevel(BCLog::I2P, BCLog::Level::Debug, "Destroying SAM session %s\n", m_session_id);
479@@ -482,9 +480,9 @@ void Session::Disconnect()
480 {
481 if (m_control_sock) {
482 if (m_session_id.empty()) {
483- Log("Destroying incomplete SAM session");
484+ LogPrintLevel(BCLog::I2P, BCLog::Level::Debug, "Destroying incomplete SAM session\n");
(reposting using the “review changes” button to appease the bot, it sent me several “request review” notifications in the last ten minutes :)
ACK modulo comments above. If the third commit is re-changing lines that are already changed in the first commit, would suggest combining them.
(reposting using the “review changes” button to appease the bot, it sent me several “request review” notifications in the last ten minutes :)
Not sure why that would happen. The bot should not react on new review comments after someone else’s A C K. (c.f. https://github.com/maflcko/DrahtBot/blob/main/webhook_features/src/features/summary_comment.rs#L308) Looking at other pull requests, this seems to be working correctly, so my guess is that this is another GitHub metatdata corruption bug, which are common?
Co-authored-by: Vasil Dimov <vd@FreeBSD.org>
Before, interruption was printed as an error. Also,
it did not log the reason when an interruption happened,
e.g. "Error accepting:".
Co-authored-by: Vasil Dimov <vd@FreeBSD.org>
201@@ -202,7 +202,11 @@ bool Session::Accept(Connection& conn)
202 return true;
203 }
204
205- Log("Error accepting%s: %s", disconnect ? " (will close the session)" : "", errmsg);
206+ if (m_interrupt) {
*m_interrupt?
CThreadInterrupt*. Thanks.
Good catch! The implicit cast to bool of CThreadInterrupt makes it easy to get such mistakes if the variable is a pointer to CThreadInterrupt or std::optional<CThreadInterrupt>. If CThreadInterrupt had an explicit method like IsInterrupted() then it would be more clear to call that one, but unfortunately it does not have such a method.
To be explicit we can do if (m_interrupt->operator bool()) { :laughing: :robot: :rofl:
Or, more seriously, because m_interrupt can never be nullptr, change it to a reference.
