From 790cda46aad250cc50049f6bfeaf40c2b5c492e7 Mon Sep 17 00:00:00 2001 From: Eduard Bagdasaryan Date: Mon, 4 Mar 2024 16:25:33 +0300 Subject: [PATCH 1/6] De-duplicate I/O error-detailing code This improvement eliminates code duplication introduced by b850f8b fix. --- src/servers/Server.cc | 19 +++++++++++-------- src/servers/Server.h | 2 ++ 2 files changed, 13 insertions(+), 8 deletions(-) diff --git a/src/servers/Server.cc b/src/servers/Server.cc index 3c4004cf511..b3d9d39bef2 100644 --- a/src/servers/Server.cc +++ b/src/servers/Server.cc @@ -173,10 +173,7 @@ Server::doClientRead(const CommIoCbParams &io) // case Comm::COMM_ERROR: default: // no other flags should ever occur debugs(33, 2, io.conn << ": got flag " << rd.flag << "; " << xstrerr(rd.xerrno)); - LogTagsErrors lte; - lte.timedout = rd.xerrno == ETIMEDOUT; - lte.aborted = !lte.timedout; // intentionally true for zero rd.xerrno - terminateAll(Error(ERR_CLIENT_GONE, SysErrorDetail::NewIfAny(rd.xerrno)), lte); + terminateWithError(ERR_READ_ERROR, rd.xerrno); return; } @@ -206,10 +203,7 @@ Server::clientWriteDone(const CommIoCbParams &io) if (io.flag) { debugs(33, 2, "bailing after a write failure: " << xstrerr(io.xerrno)); - LogTagsErrors lte; - lte.timedout = io.xerrno == ETIMEDOUT; - lte.aborted = !lte.timedout; // intentionally true for zero io.xerrno - terminateAll(Error(ERR_WRITE_ERROR, SysErrorDetail::NewIfAny(io.xerrno)), lte); + terminateWithError(ERR_WRITE_ERROR, io.xerrno); return; } @@ -217,3 +211,12 @@ Server::clientWriteDone(const CommIoCbParams &io) writeSomeData(); // maybe schedules another write } +void +Server::terminateWithError(const err_type errType, const int errNo) +{ + LogTagsErrors lte; + lte.timedout = errNo == ETIMEDOUT; + lte.aborted = !lte.timedout; // intentionally true for zero errNo + terminateAll(Error(errType, SysErrorDetail::NewIfAny(errNo)), lte); +} + diff --git a/src/servers/Server.h b/src/servers/Server.h index 10317799bcf..36bbb20c612 100644 --- a/src/servers/Server.h +++ b/src/servers/Server.h @@ -121,6 +121,8 @@ class Server : virtual public AsyncJob, public BodyProducer /// abort any pending transactions and prevent new ones (by closing) virtual void terminateAll(const Error &, const LogTagsErrors &) = 0; + /// terminateAll() after detecting read or write failure + void terminateWithError(err_type errType, int errNo); void doClientRead(const CommIoCbParams &io); void clientWriteDone(const CommIoCbParams &io); From 5a03963382c89bdff478433a67303877962f2773 Mon Sep 17 00:00:00 2001 From: Eduard Bagdasaryan Date: Mon, 4 Mar 2024 23:56:45 +0300 Subject: [PATCH 2/6] Moved de-duplication code into a dedicated LogTagsErrors::FromErrno() --- src/LogTags.cc | 9 +++++++++ src/LogTags.h | 3 +++ src/servers/Server.cc | 14 ++------------ src/servers/Server.h | 2 -- 4 files changed, 14 insertions(+), 14 deletions(-) diff --git a/src/LogTags.cc b/src/LogTags.cc index 36402056735..f12fd78f4d0 100644 --- a/src/LogTags.cc +++ b/src/LogTags.cc @@ -18,6 +18,15 @@ LogTagsErrors::update(const LogTagsErrors &other) aborted = aborted || other.aborted; } +LogTagsErrors +LogTagsErrors::FromErrno(const int errNo) +{ + LogTagsErrors lte; + lte.timedout = errNo == ETIMEDOUT; + lte.aborted = !lte.timedout; // intentionally true for zero errNo + return lte; +} + /* LogTags */ // old deprecated tag strings diff --git a/src/LogTags.h b/src/LogTags.h index d101c81a993..9a443ba1b15 100644 --- a/src/LogTags.h +++ b/src/LogTags.h @@ -17,6 +17,9 @@ class LogTagsErrors { public: + /// constructs an object from an errno produced by Comm::ReadNow() + static LogTagsErrors FromErrno(int errNo); + /// Update each of this object flags to "set" if the corresponding /// flag of the given object is set void update(const LogTagsErrors &other); diff --git a/src/servers/Server.cc b/src/servers/Server.cc index b3d9d39bef2..e12610e933c 100644 --- a/src/servers/Server.cc +++ b/src/servers/Server.cc @@ -173,7 +173,7 @@ Server::doClientRead(const CommIoCbParams &io) // case Comm::COMM_ERROR: default: // no other flags should ever occur debugs(33, 2, io.conn << ": got flag " << rd.flag << "; " << xstrerr(rd.xerrno)); - terminateWithError(ERR_READ_ERROR, rd.xerrno); + terminateAll(Error(ERR_CLIENT_GONE, SysErrorDetail::NewIfAny(rd.xerrno)), LogTagsErrors::FromErrno(rd.xerrno)); return; } @@ -203,20 +203,10 @@ Server::clientWriteDone(const CommIoCbParams &io) if (io.flag) { debugs(33, 2, "bailing after a write failure: " << xstrerr(io.xerrno)); - terminateWithError(ERR_WRITE_ERROR, io.xerrno); + terminateAll(Error(ERR_WRITE_ERROR, SysErrorDetail::NewIfAny(io.xerrno)), LogTagsErrors::FromErrno(io.xerrno)); return; } afterClientWrite(io.size); // update state writeSomeData(); // maybe schedules another write } - -void -Server::terminateWithError(const err_type errType, const int errNo) -{ - LogTagsErrors lte; - lte.timedout = errNo == ETIMEDOUT; - lte.aborted = !lte.timedout; // intentionally true for zero errNo - terminateAll(Error(errType, SysErrorDetail::NewIfAny(errNo)), lte); -} - diff --git a/src/servers/Server.h b/src/servers/Server.h index 36bbb20c612..10317799bcf 100644 --- a/src/servers/Server.h +++ b/src/servers/Server.h @@ -121,8 +121,6 @@ class Server : virtual public AsyncJob, public BodyProducer /// abort any pending transactions and prevent new ones (by closing) virtual void terminateAll(const Error &, const LogTagsErrors &) = 0; - /// terminateAll() after detecting read or write failure - void terminateWithError(err_type errType, int errNo); void doClientRead(const CommIoCbParams &io); void clientWriteDone(const CommIoCbParams &io); From 6d66bd06df490579ad5ab85a1f577fc0c1d694c3 Mon Sep 17 00:00:00 2001 From: Eduard Bagdasaryan Date: Tue, 5 Mar 2024 00:01:32 +0300 Subject: [PATCH 3/6] Removed an out-of-scope change --- src/servers/Server.cc | 1 + 1 file changed, 1 insertion(+) diff --git a/src/servers/Server.cc b/src/servers/Server.cc index e12610e933c..5cf8517ee92 100644 --- a/src/servers/Server.cc +++ b/src/servers/Server.cc @@ -210,3 +210,4 @@ Server::clientWriteDone(const CommIoCbParams &io) afterClientWrite(io.size); // update state writeSomeData(); // maybe schedules another write } + From 3c246c021dcd5df17a9981476e839943e3472e43 Mon Sep 17 00:00:00 2001 From: Eduard Bagdasaryan Date: Tue, 5 Mar 2024 01:02:07 +0300 Subject: [PATCH 4/6] Fixed a description --- src/LogTags.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/LogTags.h b/src/LogTags.h index 9a443ba1b15..fe6f658b354 100644 --- a/src/LogTags.h +++ b/src/LogTags.h @@ -17,7 +17,7 @@ class LogTagsErrors { public: - /// constructs an object from an errno produced by Comm::ReadNow() + /// constructs an object matching errno(3) of a failed I/O call static LogTagsErrors FromErrno(int errNo); /// Update each of this object flags to "set" if the corresponding From 2dc49ca9143c7d84f9d863742cb2a0cbe9c89fee Mon Sep 17 00:00:00 2001 From: Eduard Bagdasaryan Date: Wed, 6 Mar 2024 16:43:28 +0300 Subject: [PATCH 5/6] Reduced code duplication in FwdState::updateAleWithFinalError() --- src/FwdState.cc | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/src/FwdState.cc b/src/FwdState.cc index 509127fc525..abae1d6a5ac 100644 --- a/src/FwdState.cc +++ b/src/FwdState.cc @@ -240,11 +240,7 @@ FwdState::updateAleWithFinalError() if (!err || !al) return; - LogTagsErrors lte; - if (err->xerrno == ETIMEDOUT || err->type == ERR_READ_TIMEOUT) - lte.timedout = true; - else if (err->type != ERR_NONE) - lte.aborted = true; + const auto lte = LogTagsErrors::FromErrno(err->type == ERR_READ_TIMEOUT ? ETIMEDOUT : err->xerrno); al->cache.code.err.update(lte); if (!err->detail) { static const auto d = MakeNamedErrorDetail("WITH_SERVER"); From ffb32515eefd472a26eb554120d937d3d505201c Mon Sep 17 00:00:00 2001 From: Alex Rousskov Date: Wed, 6 Mar 2024 15:50:14 -0500 Subject: [PATCH 6/6] fixup: Use parenthesis to avoid arguing about them Co-authored-by: Amos Jeffries --- src/LogTags.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/LogTags.cc b/src/LogTags.cc index f12fd78f4d0..548023fb746 100644 --- a/src/LogTags.cc +++ b/src/LogTags.cc @@ -22,7 +22,7 @@ LogTagsErrors LogTagsErrors::FromErrno(const int errNo) { LogTagsErrors lte; - lte.timedout = errNo == ETIMEDOUT; + lte.timedout = (errNo == ETIMEDOUT); lte.aborted = !lte.timedout; // intentionally true for zero errNo return lte; }