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

Jump to bottom

fix: endless matching loop #199

Merged
merged 1 commit into from
Jun 28, 2024
Merged

fix: endless matching loop #199

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
10 changes: 6 additions & 4 deletions layer4/routes.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,15 +108,14 @@ func (routes RouteList) Compile(logger *zap.Logger, matchingTimeout time.Duratio
if err != nil {
return err
}
isFirstPrefetch := true
for { // retry prefetching and matching routes until timeout

for i := 0; i < 10000; i++ { // retry prefetching and matching routes until timeout

// Do not call prefetch if this is the first loop iteration and there already is some data available,
// since this means we are at the start of a subroute handler and previous prefetch calls likely already fetched all bytes available from the client.
// Which means it would block the subroute handler. In the second iteration (if no subroute routes match) blocking is the correct behaviour.
if !isFirstPrefetch || cx.buf == nil || len(cx.buf[cx.offset:]) == 0 {
if i != 0 || cx.buf == nil || len(cx.buf[cx.offset:]) == 0 {
mholt marked this conversation as resolved.
Show resolved Hide resolved
err = cx.prefetch()
isFirstPrefetch = false
if err != nil {
logFunc := logger.Error
if errors.Is(err, os.ErrDeadlineExceeded) {
Expand Down Expand Up @@ -184,5 +183,8 @@ func (routes RouteList) Compile(logger *zap.Logger, matchingTimeout time.Duratio
}
}
}

logger.Error("matching connection", zap.String("remote", cx.RemoteAddr().String()), zap.Error(errors.New("number of prefetch calls exhausted")))
return nil
Copy link
Owner

Choose a reason for hiding this comment

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

I wonder if we should return the error here. But this is also probably fine. Any thoughts?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's that way because I wanted to differentiate errors during matching vs errors from the handlers. "matching connection" vs. "handling connection". But I agree it looks bad/wrong.
We could swap the positions. Log "matching connection" in server.go#L168 and "handling connection" in routes.go#L173. Then the HandlerFunc in Compile could return errors directly everywhere.
But the logic for only logging with warning level for ErrDeadlineExceeded should probably be kept in the compiled function. Otherwise it must be duplicated to all other places that call Handle on the compiled route. Like the listener wrapper or the subroute handler.

Copy link
Owner

Choose a reason for hiding this comment

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

Yeah, so it's in a function that acts as a handler representing the entire route, which is both matching and handling. That's a good question, I guess, whether we should treat their errors separately or differently. A matcher should only return an error if a decision (true/false) couldn't be reached; that should be rare, and notable, since it indicates a problem with the connection. Maybe we should treat their errors equally?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sorry for the delayed response. For now, I would leave it as it is. I think the different messages are useful to differentiate errors during the routing/matching phase and caddy actually knowing what to do with the connection. Since errors during matching are likely caused by clients sending garbage, disconnecting early or configuration problems. But errors during handing likely indicate real problems for real/valid users.

})
}
Loading