diff --git a/src/StoreClient.h b/src/StoreClient.h index 484b413..b523cc0 100644 --- a/src/StoreClient.h +++ b/src/StoreClient.h @@ -203,7 +203,7 @@ private: public: struct Callback { - Callback ():callback_handler(NULL), callback_data(NULL) {} + Callback() = default; Callback (STCB *, void *); @@ -212,8 +212,8 @@ public: /// delivery to the STCB callback_handler. bool pending() const; - STCB *callback_handler; - void *callback_data; + STCB *callback_handler = nullptr; ///< where to deliver the answer + CallbackData cbData; ///< the first STCB callback parameter CodeContextPointer codeContext; ///< Store client context /// a scheduled asynchronous finishCallback() call (or nil) diff --git a/src/store_client.cc b/src/store_client.cc index d8ae028..e7144b4 100644 --- a/src/store_client.cc +++ b/src/store_client.cc @@ -188,14 +188,12 @@ store_client::finishCallback() ++answers; STCB *temphandler = _callback.callback_handler; - void *cbdata = _callback.callback_data; - _callback = Callback(NULL, NULL); + const auto cbdata = _callback.cbData.validDone(); + _callback = Callback(); copyInto.data = NULL; - if (cbdataReferenceValid(cbdata)) + if (cbdata) temphandler(cbdata, result); - - cbdataReferenceDone(cbdata); } store_client::store_client(StoreEntry *e) : @@ -255,7 +253,7 @@ store_client::copy(StoreEntry * anEntry, #endif assert(!_callback.pending()); - _callback = Callback (callback_fn, cbdataReference(data)); + _callback = Callback(callback_fn, data); copyInto.data = copyRequest.data; copyInto.length = copyRequest.length; copyInto.offset = copyRequest.offset; @@ -1129,7 +1127,7 @@ store_client::dumpStats(MemBuf * output, int clientNumber) const if (_callback.pending()) return; - output->appendf("\tClient #%d, %p\n", clientNumber, _callback.callback_data); + output->appendf("\tClient #%d, %p\n", clientNumber, this); output->appendf("\t\tcopy_offset: %" PRId64 "\n", copyInto.offset); output->appendf("\t\tcopy_size: %" PRIuSIZE "\n", copyInto.length); output->append("\t\tflags:", 8); @@ -1154,7 +1152,7 @@ store_client::Callback::pending() const store_client::Callback::Callback(STCB *function, void *data): callback_handler(function), - callback_data(data), + cbData(data), codeContext(CodeContext::Current()) { } diff --git a/src/tunnel.cc b/src/tunnel.cc index 4fc5abd..2d37d2e 100644 --- a/src/tunnel.cc +++ b/src/tunnel.cc @@ -273,6 +273,7 @@ private: /// resumes operations after the (possibly failed) HTTP CONNECT exchange void tunnelEstablishmentDone(Http::TunnelerAnswer &answer); + void finishWritingAndDelete(Connection &); void deleteThis(); void cancelStep(const char *reason); @@ -319,7 +320,9 @@ TunnelStateData::serverClosed() { server.noteClosure(); - retryOrBail(__FUNCTION__); + request->hier.stopPeerClock(false); + + finishWritingAndDelete(client); } /// TunnelStateData::clientClosed() wrapper @@ -334,12 +337,48 @@ void TunnelStateData::clientClosed() { client.noteClosure(); + finishWritingAndDelete(server); +} +/// Gracefully handles non-retriable connection closure. If necessary, either +/// starts closing the given connection or waits for its pending write to +/// finish. Otherwise, immediately destroys the tunnel object. +/// \prec The other Connection is not open. +void +TunnelStateData::finishWritingAndDelete(Connection &remainingConnection) +{ if (noConnections()) return deleteThis(); - if (!server.writer) - server.conn->close(); + // XXX: The code below should precede the noConnections() check above. When + // there is no writer, we should trigger an immediate noConnections() + // outcome instead of waiting for an asynchronous call to our own closure + // callback (that will call this method again). We should not move this code + // until a noConnections() outcome guarantees a nil writer because such a + // move will unnecessary delay deleteThis(). + + if (remainingConnection.writer) { + debugs(26, 5, "waiting to finish writing to " << remainingConnection.conn); + // the write completion callback must close its remainingConnection + // after noticing that the other connection is gone + return; + } + + // XXX: Stop abusing connection closure callback for terminating tunneling + // in cases like this, where our code knows that tunneling must end. The + // closure callback should be dedicated to handling rare connection closures + // originated _outside_ of TunnelStateData (e.g., during shutdown). In all + // other cases, our own close()-calling code must detail the + // closure-triggering error (if any) _and_ clear all callbacks: Our code + // does not need to be (asynchronously) notified of the closure that it + // itself has initiated! Until that (significant) refactoring, + // serverClosed() and clientClosed() callbacks will continue to mishandle + // those rare closures as regular ones, and access.log records will continue + // to lack some tunneling error indicators/details. + // + // This asynchronous close() leads to another finishWritingAndDelete() call + // but with true noConnections() that finally triggers deleteThis(). + remainingConnection.conn->close(); } /// destroys the tunnel (after performing potentially-throwing cleanup) @@ -451,14 +490,7 @@ TunnelStateData::retryOrBail(const char *context) return sendError(savedError, bailDescription ? bailDescription : context); *status_ptr = savedError->httpStatus; - if (noConnections()) - return deleteThis(); - - // This is a "Comm::IsConnOpen(client.conn) but !canSendError" case. - // Closing the connection (after finishing writing) is the best we can do. - if (!client.writer) - client.conn->close(); - // else writeClientDone() must notice a closed server and close the client + finishWritingAndDelete(client); } int