DelayId::operator bool() should account for cache_peer no-delay (#2121)

Without this check, a DelayId::operator bool() caller could incorrectly
limit reading from a cache_peer that has been configured to ignore delay
pools. Luckily, all existing code paths that use this operator checked
the markedAsNoDelay flag, avoiding this problem:

* DelayId::bytesWanted() and DelayId::bytesIn() checked markedAsNoDelay.

* While MemObject::delayRead() itself did not check markedAsNoDelay, the
  method is not called when markedAsNoDelay is true. The analysis of the
  corresponding code paths is complex, especially for HttpStateData! We
  also call expensive mostBytesAllowed() several times on these paths.
  Refactoring this code is outside this API safety improvement scope.

This change helps avoid accidental/unwanted read delays during future
code changes.

diff --git a/src/DelayId.cc b/src/DelayId.cc
index 2e85f0eb063d5a50068421961afc52f72ef9fc61..9f97ed6758d73f85c094ceafee8ceb64a4a476aa 100644 (file)
--- a/src/DelayId.cc
+++ b/src/DelayId.cc
@@ -60,7 +60,7 @@ DelayId::operator == (DelayId const &rhs) const
 
 DelayId::operator bool() const
 {
-    return pool_ || compositeId.getRaw();
+    return (pool_ || compositeId.getRaw()) && !markedAsNoDelay;
 }
 
 /* create a delay Id for a given request */
@@ -127,7 +127,7 @@ DelayId::bytesWanted(int minimum, int maximum) const
 {
     /* unlimited */
 
-    if (! (*this) || markedAsNoDelay)
+    if (! (*this))
         return max(minimum, maximum);
 
     /* limited */
@@ -150,9 +150,6 @@ DelayId::bytesIn(int qty)
     if (! (*this))
         return;
 
-    if (markedAsNoDelay)
-        return;
-
     assert ((unsigned short)(pool() - 1) != 0xFFFF);
 
     if (compositeId != nullptr)

diff --git a/src/DelayId.h b/src/DelayId.h
index d010e18ccb9473d8db6765a7299eb14610e2f2f9..66036101f5872acfe555e0b18fd8435020b50962 100644 (file)
--- a/src/DelayId.h
+++ b/src/DelayId.h
@@ -31,7 +31,13 @@ public:
     DelayIdComposite::Pointer const compositePosition() const;
     void compositePosition(const DelayIdComposite::Pointer &);
     bool operator == (DelayId const &rhs) const;
+
+    /// Whether we may delay reading. This operator is meant to be used as an
+    /// optimization that helps avoid more expensive bytesWanted() computations.
+    /// \retval false if bytesWanted() called with a positive maximum limit
+    /// parameter will never return zero
     operator bool() const;
+
     int bytesWanted(int min, int max) const;
     void bytesIn (int qty);
     void setNoDelay(bool const);