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.

Sign up for GitHub

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

De-duplicate I/O error-detailing code #247

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

De-duplicate I/O error-detailing code #247

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
790cda4
De-duplicate I/O error-detailing code
eduard-bagdasaryan Mar 4, 2024
5a03963
Moved de-duplication code into a dedicated LogTagsErrors::FromErrno()
eduard-bagdasaryan Mar 4, 2024
6d66bd0
Removed an out-of-scope change
eduard-bagdasaryan Mar 4, 2024
3c246c0
Fixed a description
eduard-bagdasaryan Mar 4, 2024
1e486a0
Merged from master
eduard-bagdasaryan Mar 5, 2024
2dc49ca
Reduced code duplication in FwdState::updateAleWithFinalError()
eduard-bagdasaryan Mar 6, 2024
ffb3251
fixup: Use parenthesis to avoid arguing about them
rousskov Mar 6, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 11 additions & 8 deletions src/servers/Server.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
rousskov marked this conversation as resolved.
Show resolved Hide resolved
return;
}

Expand Down Expand Up @@ -206,14 +203,20 @@ 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);
Copy link

Choose a reason for hiding this comment

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

I would like to avoid adding a second terminateX() method just to encapsulate Error and LogTagsErrors creation code, especially when that encapsulation moves us back from a flexible/safe/explicit ErrorDetail API to none-of-those-things int-is-errno API. Adding terminateX() may also encourage creation of terminateY() and terminateZ() to accommodate other error detail construction cases, and we do not want that kind of API fan-out.

I realize that part of the appeal here is how LogTagsErrors objects are initialized from errno. I am pretty sure that code should eventually disappear because we can compute the same log tags based on the information stored in Error. However, polishing that aspect will result in more changes than I would like to see in this particular PR.

Instead of adding terminateWithError(), please add and use a LogTagsErrors::FromErrno() static method that returns a properly configured LogTagsErrors object. Doing so will arguably leave some of the logic duplicated, but I think it is better than adding a competing terminateX() interface with raw errno argument.

Suggested change
terminateWithError(ERR_WRITE_ERROR, io.xerrno);
terminateAll(Error(ERR_WRITE_ERROR, SysErrorDetail::NewIfAny(io.xerrno)), LogTagsErrors::FromErrno(io.xerrno));

Copy link
Author

Choose a reason for hiding this comment

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

Done (5a03963).

return;
Copy link

Choose a reason for hiding this comment

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

Is it possible to do something about ERR_READ_TIMEOUT case (e.g., make sure it always has ETIMEDOUT) so that we can refactor FwdState::updateAleWithFinalError() to use the new method as well?

Copy link
Author

Choose a reason for hiding this comment

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

I don't think that ERR_READ_TIMEOUT will be always paired with ETIMEDOUT. ERR_READ_TIMEOUT is set in Ftp::Client::timeout() and HttpStateData::httpTimeout() - this code reflects Squid-configuration level timeouts whereas ETIMEDOUT reflects TCP-level timeouts.

Copy link

Choose a reason for hiding this comment

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

I do not disagree, but LogTagsErrors do need to signal a TIMEOUT suffix for both kinds of timeouts. Should FwdState::updateAleWithFinalError() do something like the following to simplify that code logic and reduce its duplication?

const auto lte = LogTagsErrors::FromErrno(err->type == ERR_READ_TIMEOUT ? ETIMEOUT : err->xerrno);

The above sketch effectively removes the ERR_NONE check in the existing code. I could not quickly figure out why we added that part of the check in recent commit 21f9015. Can you?

Copy link
Author

Choose a reason for hiding this comment

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

why we added that part of the check in recent commit 21f9015

No, I could not find an explanation. It was added in the initial PR186 commit. Maybe some of ErrorState() creators called constructed it with ERR_NONE then? I looked through these callers now and could not find any. Anyway, I think it is better to reduce the code duplication as you suggested.

}

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);
}

2 changes: 2 additions & 0 deletions src/servers/Server.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);

Expand Down
Loading