From a3f1bb546fec72b07f11f69934e6894696fea57a Mon Sep 17 00:00:00 2001 From: James Turner Date: Thu, 14 Apr 2016 09:15:57 +0100 Subject: [PATCH] HTTP request cancellation - replaces abort() with something more structured. --- simgear/canvas/elements/CanvasImage.cxx | 7 +- simgear/io/HTTPClient.cxx | 120 ++++++++++++++++++++---- simgear/io/HTTPClient.hxx | 2 + simgear/io/HTTPFileRequest.cxx | 2 - simgear/io/HTTPRequest.cxx | 18 ++-- simgear/io/HTTPRequest.hxx | 12 +-- simgear/io/test_HTTP.cxx | 68 ++++++++++++++ simgear/package/Install.cxx | 2 +- simgear/package/Root.cxx | 14 +++ simgear/package/Root.hxx | 5 + 10 files changed, 203 insertions(+), 47 deletions(-) diff --git a/simgear/canvas/elements/CanvasImage.cxx b/simgear/canvas/elements/CanvasImage.cxx index 99d332c4..152cb112 100644 --- a/simgear/canvas/elements/CanvasImage.cxx +++ b/simgear/canvas/elements/CanvasImage.cxx @@ -155,8 +155,9 @@ namespace canvas //---------------------------------------------------------------------------- Image::~Image() { - if( _http_request ) - _http_request->abort("image destroyed"); + if( _http_request ) { + Canvas::getSystemAdapter()->getHTTPClient()->cancelRequest(_http_request, "image destroyed"); + } } //---------------------------------------------------------------------------- @@ -618,7 +619,7 @@ namespace canvas // Abort pending request if( _http_request ) { - _http_request->abort("setting new image"); + Canvas::getSystemAdapter()->getHTTPClient()->cancelRequest(_http_request, "setting new image"); _http_request.reset(); } diff --git a/simgear/io/HTTPClient.cxx b/simgear/io/HTTPClient.cxx index 03302e51..d5ef1ea5 100644 --- a/simgear/io/HTTPClient.cxx +++ b/simgear/io/HTTPClient.cxx @@ -78,7 +78,6 @@ class Client::ClientPrivate public: #if defined(ENABLE_CURL) CURLM* curlMulti; - bool haveActiveRequests; void createCurlMulti() { @@ -94,6 +93,9 @@ public: } + + typedef std::map RequestCurlMap; + RequestCurlMap requests; #else NetChannelPoller poller; // connections by host (potentially more than one) @@ -205,6 +207,8 @@ public: // closing of the connection from the server side when getting the body, bool canCloseState = (state == STATE_GETTING_BODY); + bool isCancelling = (state == STATE_CANCELLING); + if (canCloseState && activeRequest) { // check bodyTransferSize matches how much we actually transferred if (bodyTransferSize > 0) { @@ -229,7 +233,7 @@ public: sentRequests.erase(sentRequests.begin()); } - if (activeRequest) { + if (activeRequest && !isCancelling) { activeRequest->setFailure(500, "server closed connection"); // remove the failed request from sentRequests, so it does // not get restored @@ -262,6 +266,39 @@ public: tryStartNextRequest(); } + void cancelRequest(const Request_ptr& r) + { + RequestList::iterator it = std::find(sentRequests.begin(), + sentRequests.end(), r); + if (it != sentRequests.end()) { + sentRequests.erase(it); + + if ((r == activeRequest) || !activeRequest) { + // either the cancelling request is active, or we're in waiting + // for response state - close now + setState(STATE_CANCELLING); + close(); + + setState(STATE_CLOSED); + activeRequest = NULL; + _contentDecoder.reset(); + } else if (activeRequest) { + SG_LOG(SG_IO, SG_INFO, "con:" << _connectionId << " cancelling non-active: " << r->url()); + + // has been sent but not active, let the active finish and + // then close. Otherwise cancelling request #2 would mess up + // active transfer #1 + activeRequest->setCloseAfterComplete(); + } + } // of request has been sent + + // simpler case, not sent yet just remove from the queue + it = std::find(queuedRequests.begin(), queuedRequests.end(), r); + if (it != queuedRequests.end()) { + queuedRequests.erase(it); + } + } + void beginResponse() { assert(!sentRequests.empty()); @@ -528,6 +565,7 @@ private: STATE_GETTING_CHUNKED_BYTES, STATE_GETTING_TRAILER, STATE_SOCKET_ERROR, + STATE_CANCELLING, ///< cancelling an acitve request STATE_CLOSED ///< connection should be closed now }; @@ -787,34 +825,43 @@ void Client::update(int waitTimeout) #if defined(ENABLE_CURL) int remainingActive, messagesInQueue; curl_multi_perform(d->curlMulti, &remainingActive); - d->haveActiveRequests = (remainingActive > 0); CURLMsg* msg; while ((msg = curl_multi_info_read(d->curlMulti, &messagesInQueue))) { if (msg->msg == CURLMSG_DONE) { - Request* req; + Request* rawReq = 0; CURL *e = msg->easy_handle; - curl_easy_getinfo(e, CURLINFO_PRIVATE, &req); + curl_easy_getinfo(e, CURLINFO_PRIVATE, &rawReq); + + // ensure request stays valid for the moment + // eg if responseComplete cancels us + Request_ptr req(rawReq); long responseCode; curl_easy_getinfo(e, CURLINFO_RESPONSE_CODE, &responseCode); + // remove from the requests map now, + // in case the callbacks perform a cancel. We'll use + // the absence from the request dict in cancel to avoid + // a double remove + ClientPrivate::RequestCurlMap::iterator it = d->requests.find(req); + assert(it != d->requests.end()); + assert(it->second == e); + d->requests.erase(it); + if (msg->data.result == 0) { req->responseComplete(); } else { - fprintf(stderr, "Result: %d - %s\n", - msg->data.result, curl_easy_strerror(msg->data.result)); + SG_LOG(SG_IO, SG_WARN, "CURL Result:" << msg->data.result << " " << curl_easy_strerror(msg->data.result)); req->setFailure(msg->data.result, curl_easy_strerror(msg->data.result)); } curl_multi_remove_handle(d->curlMulti, e); - - // balance the reference we take in makeRequest - SGReferenced::put(req); curl_easy_cleanup(e); - } - else { - SG_LOG(SG_IO, SG_ALERT, "CurlMSG:" << msg->msg); + } else { + // should never happen since CURLMSG_DONE is the only code + // defined! + SG_LOG(SG_IO, SG_ALERT, "unknown CurlMSG:" << msg->msg); } } // of curl message processing loop SGTimeStamp::sleepForMSec(waitTimeout); @@ -880,11 +927,14 @@ void Client::makeRequest(const Request_ptr& r) r->_client = this; #if defined(ENABLE_CURL) + ClientPrivate::RequestCurlMap::iterator rit = d->requests.find(r); + assert(rit == d->requests.end()); + CURL* curlRequest = curl_easy_init(); curl_easy_setopt(curlRequest, CURLOPT_URL, r->url().c_str()); - // manually increase the ref count of the request - SGReferenced::get(r.get()); + d->requests[r] = curlRequest; + curl_easy_setopt(curlRequest, CURLOPT_PRIVATE, r.get()); // disable built-in libCurl progress feedback curl_easy_setopt(curlRequest, CURLOPT_NOPROGRESS, 1); @@ -950,9 +1000,9 @@ void Client::makeRequest(const Request_ptr& r) } curl_multi_add_handle(d->curlMulti, curlRequest); - d->haveActiveRequests = true; -// FIXME - premature? +// this seems premature, but we don't have a callback from Curl we could +// use to trigger when the requst is actually sent. r->requestStart(); #else @@ -1034,6 +1084,33 @@ void Client::makeRequest(const Request_ptr& r) #endif } +void Client::cancelRequest(const Request_ptr &r, std::string reason) +{ +#if defined(ENABLE_CURL) + ClientPrivate::RequestCurlMap::iterator it = d->requests.find(r); + if(it == d->requests.end()) { + // already being removed, presumably inside ::update() + // nothing more to do + return; + } + + CURLMcode err = curl_multi_remove_handle(d->curlMulti, it->second); + assert(err == CURLM_OK); + + // clear the request pointer form the curl-easy object + curl_easy_setopt(it->second, CURLOPT_PRIVATE, 0); + + curl_easy_cleanup(it->second); + d->requests.erase(it); +#else + ConnectionDict::iterator it = d->connections.begin(); + for (; it != d->connections.end(); ++it) { + (it->second)->cancelRequest(r); + } +#endif + r->setFailure(-1, reason); +} + //------------------------------------------------------------------------------ FileRequestRef Client::save( const std::string& url, const std::string& filename ) @@ -1088,7 +1165,7 @@ void Client::setProxy( const std::string& proxy, bool Client::hasActiveRequests() const { #if defined(ENABLE_CURL) - return d->haveActiveRequests; + return !d->requests.empty(); #else ConnectionDict::const_iterator it = d->connections.begin(); for (; it != d->connections.end(); ++it) { @@ -1201,7 +1278,12 @@ size_t Client::requestHeaderCallback(char *rawBuffer, size_t size, size_t nitems void Client::debugDumpRequests() { #if defined(ENABLE_CURL) - + SG_LOG(SG_IO, SG_INFO, "== HTTP request dump"); + ClientPrivate::RequestCurlMap::iterator it = d->requests.begin(); + for (; it != d->requests.end(); ++it) { + SG_LOG(SG_IO, SG_INFO, "\t" << it->first->url()); + } + SG_LOG(SG_IO, SG_INFO, "=="); #else SG_LOG(SG_IO, SG_INFO, "== HTTP connection dump"); ConnectionDict::iterator it = d->connections.begin(); diff --git a/simgear/io/HTTPClient.hxx b/simgear/io/HTTPClient.hxx index 00682c4c..a535b5b5 100644 --- a/simgear/io/HTTPClient.hxx +++ b/simgear/io/HTTPClient.hxx @@ -49,6 +49,8 @@ public: void makeRequest(const Request_ptr& r); + void cancelRequest(const Request_ptr& r, std::string reason = std::string()); + /** * Download a resource and save it to a file. * diff --git a/simgear/io/HTTPFileRequest.cxx b/simgear/io/HTTPFileRequest.cxx index 81ddb012..46021625 100644 --- a/simgear/io/HTTPFileRequest.cxx +++ b/simgear/io/HTTPFileRequest.cxx @@ -59,8 +59,6 @@ namespace HTTP SG_WARN, "HTTP::FileRequest: failed to open file '" << _filename << "'" ); - - abort("Failed to open file."); } } diff --git a/simgear/io/HTTPRequest.cxx b/simgear/io/HTTPRequest.cxx index 63bae965..6b06785a 100644 --- a/simgear/io/HTTPRequest.cxx +++ b/simgear/io/HTTPRequest.cxx @@ -366,23 +366,17 @@ void Request::setReadyState(ReadyState state) } //------------------------------------------------------------------------------ -void Request::abort() -{ - abort("Request aborted."); -} - -//---------------------------------------------------------------------------- -void Request::abort(const std::string& reason) +bool Request::closeAfterComplete() const { - setFailure(-1, reason); - _willClose = true; + // for non HTTP/1.1 connections, assume server closes + return _willClose || (_responseVersion != HTTP_1_1); } //------------------------------------------------------------------------------ -bool Request::closeAfterComplete() const + +void Request::setCloseAfterComplete() { - // for non HTTP/1.1 connections, assume server closes - return _willClose || (_responseVersion != HTTP_1_1); + _willClose = true; } //------------------------------------------------------------------------------ diff --git a/simgear/io/HTTPRequest.hxx b/simgear/io/HTTPRequest.hxx index 7c06fc8e..0def0888 100644 --- a/simgear/io/HTTPRequest.hxx +++ b/simgear/io/HTTPRequest.hxx @@ -199,16 +199,6 @@ public: ReadyState readyState() const { return _ready_state; } - /** - * Request aborting this request. - */ - void abort(); - - /** - * Request aborting this request and specify the reported reaseon for it. - */ - void abort(const std::string& reason); - bool closeAfterComplete() const; bool isComplete() const; @@ -246,6 +236,8 @@ private: void processBodyBytes(const char* s, int n); void setReadyState(ReadyState state); + void setCloseAfterComplete(); + Client* _client; // HTTP client we're active on std::string _method; diff --git a/simgear/io/test_HTTP.cxx b/simgear/io/test_HTTP.cxx index 4787a995..44a6c638 100644 --- a/simgear/io/test_HTTP.cxx +++ b/simgear/io/test_HTTP.cxx @@ -698,6 +698,74 @@ cout << "testing proxy close" << endl; COMPARE(tr->responseBytesReceived(), 0); } + // test cancel + { + cout << "cancel request" << endl; + testServer.resetConnectCount(); + cl.clearAllConnections(); + + cl.setProxy("", 80); + TestRequest* tr = new TestRequest("http://localhost:2000/test1"); + HTTP::Request_ptr own(tr); + cl.makeRequest(tr); + + TestRequest* tr2 = new TestRequest("http://localhost:2000/testLorem"); + HTTP::Request_ptr own2(tr2); + cl.makeRequest(tr2); + + TestRequest* tr3 = new TestRequest("http://localhost:2000/test1"); + HTTP::Request_ptr own3(tr3); + cl.makeRequest(tr3); + + cl.cancelRequest(tr, "my reason 1"); + + cl.cancelRequest(tr2, "my reason 2"); + + waitForComplete(&cl, tr3); + + COMPARE(tr->responseCode(), -1); + COMPARE(tr2->responseReason(), "my reason 2"); + + COMPARE(tr3->responseLength(), strlen(BODY1)); + COMPARE(tr3->responseBytesReceived(), strlen(BODY1)); + COMPARE(tr3->bodyData, string(BODY1)); + } + + // test cancel + { + cout << "cancel middle request" << endl; + testServer.resetConnectCount(); + cl.clearAllConnections(); + + cl.setProxy("", 80); + TestRequest* tr = new TestRequest("http://localhost:2000/test1"); + HTTP::Request_ptr own(tr); + cl.makeRequest(tr); + + TestRequest* tr2 = new TestRequest("http://localhost:2000/testLorem"); + HTTP::Request_ptr own2(tr2); + cl.makeRequest(tr2); + + TestRequest* tr3 = new TestRequest("http://localhost:2000/test1"); + HTTP::Request_ptr own3(tr3); + cl.makeRequest(tr3); + + cl.cancelRequest(tr2, "middle request"); + + waitForComplete(&cl, tr3); + + COMPARE(tr->responseCode(), 200); + COMPARE(tr->responseLength(), strlen(BODY1)); + COMPARE(tr->responseBytesReceived(), strlen(BODY1)); + COMPARE(tr->bodyData, string(BODY1)); + + COMPARE(tr2->responseCode(), -1); + + COMPARE(tr3->responseLength(), strlen(BODY1)); + COMPARE(tr3->responseBytesReceived(), strlen(BODY1)); + COMPARE(tr3->bodyData, string(BODY1)); + } + { cout << "get-during-response-send" << endl; cl.clearAllConnections(); diff --git a/simgear/package/Install.cxx b/simgear/package/Install.cxx index a240be17..ae6ccb5b 100644 --- a/simgear/package/Install.cxx +++ b/simgear/package/Install.cxx @@ -409,7 +409,7 @@ size_t Install::downloadedBytes() const void Install::cancelDownload() { if (m_download.valid()) { - m_download->abort("User cancelled download"); + m_package->catalog()->root()->cancelHTTPRequest(m_download, "User cancelled download"); } if (m_revision == 0) { diff --git a/simgear/package/Root.cxx b/simgear/package/Root.cxx index c158dbdd..78507653 100644 --- a/simgear/package/Root.cxx +++ b/simgear/package/Root.cxx @@ -226,6 +226,20 @@ void Root::makeHTTPRequest(HTTP::Request *req) d->httpPendingRequests.push_back(req); } +void Root::cancelHTTPRequest(HTTP::Request *req, const std::string &reason) +{ + if (d->http) { + d->http->cancelRequest(req, reason); + } + + std::deque::iterator it = std::find(d->httpPendingRequests.begin(), + d->httpPendingRequests.end(), + req); + if (it != d->httpPendingRequests.end()) { + d->httpPendingRequests.erase(it); + } +} + Root::Root(const SGPath& aPath, const std::string& aVersion) : d(new RootPrivate) { diff --git a/simgear/package/Root.hxx b/simgear/package/Root.hxx index eadfac88..69ac4804 100644 --- a/simgear/package/Root.hxx +++ b/simgear/package/Root.hxx @@ -81,6 +81,11 @@ public: * set yet. */ void makeHTTPRequest(HTTP::Request* req); + + /** + * Cancel an HTTP request. + */ + void cancelHTTPRequest(HTTP::Request* req, const std::string& reason); /** * The catalog XML/property version in use. This is used to make incomaptible -- 2.39.5