From b0d986a9d956911fc03d6dddc7c96f458c02b875 Mon Sep 17 00:00:00 2001 From: Visa Putkinen Date: Tue, 26 Nov 2013 23:00:42 +0200 Subject: [PATCH 1/3] Add delay before auth token refresh This is sometimes necessary to prevent too frequent requests. --- libgrive/src/protocol/AuthAgent.cc | 1 + 1 file changed, 1 insertion(+) diff --git a/libgrive/src/protocol/AuthAgent.cc b/libgrive/src/protocol/AuthAgent.cc index 745f274c..6a329ceb 100644 --- a/libgrive/src/protocol/AuthAgent.cc +++ b/libgrive/src/protocol/AuthAgent.cc @@ -152,6 +152,7 @@ bool AuthAgent::CheckRetry( long response ) Log( "resquest failed due to auth token expired: %1%. refreshing token", response, log::warning ) ; + os::Sleep( 5 ) ; m_auth.Refresh() ; return true ; } From a11eae8a875e579c7ad25380512b30084d8c0bb1 Mon Sep 17 00:00:00 2001 From: Visa Putkinen Date: Mon, 25 Nov 2013 00:07:27 +0200 Subject: [PATCH 2/3] Fix hang when upload receives HTTP 500 When an uploading PUT request got a HTTP 500 as reponse, grive hanged forever inside libcurl. This was because the File parameter was not rewound to 0 position on retry. The XmlResponse had to be cleared as well. Rewinding the File and clearing the XmlResponse were not enough to fix the problem, because when retrying after 500, HTTP 410 Gone or 412 Precondition failed is often received, and CheckHttpResponse would throw an exception that crashes grive. Therefore, I implemented a retry logic to Resource::Upload that retries the whole upload transaction if 410 or 412 was received. --- libgrive/src/drive/Resource.cc | 54 +++++++++++++++++++++--------- libgrive/src/http/XmlResponse.cc | 5 +++ libgrive/src/protocol/AuthAgent.cc | 20 +++++++++-- 3 files changed, 61 insertions(+), 18 deletions(-) diff --git a/libgrive/src/drive/Resource.cc b/libgrive/src/drive/Resource.cc index 4010cab2..fdb6d641 100644 --- a/libgrive/src/drive/Resource.cc +++ b/libgrive/src/drive/Resource.cc @@ -36,6 +36,7 @@ #include "xml/Node.hh" #include "xml/NodeSet.hh" #include "xml/String.hh" +#include "xml/TreeBuilder.hh" #include #include @@ -599,23 +600,44 @@ bool Resource::Upload( % xml::Escape(m_name) ).str() ; - http::StringResponse str ; - if ( post ) - http->Post( link, meta, &str, hdr ) ; - else - http->Put( link, meta, &str, hdr ) ; - - http::Header uphdr ; - uphdr.Add( "Expect:" ) ; - uphdr.Add( "Accept:" ) ; + bool retrying=false; + while ( true ) { + if ( retrying ) { + file.Seek( 0, SEEK_SET ); + os::Sleep( 5 ); + } - // the content upload URL is in the "Location" HTTP header - std::string uplink = http->RedirLocation() ; - http::XmlResponse xml ; - - http->Put( uplink, &file, &xml, uphdr ) ; - AssignIDs( Entry( xml.Response() ) ) ; - m_mtime = Entry(xml.Response()).MTime(); + http::StringResponse str ; + if ( post ) + http->Post( link, meta, &str, hdr ) ; + else + http->Put( link, meta, &str, hdr ) ; + + http::Header uphdr ; + uphdr.Add( "Expect:" ) ; + uphdr.Add( "Accept:" ) ; + + // the content upload URL is in the "Location" HTTP header + std::string uplink = http->RedirLocation() ; + http::XmlResponse xml ; + + long http_code = 0; + http_code = http->Put( uplink, &file, &xml, uphdr ) ; + + if ( http_code == 410 || http_code == 412 ) { + Log( "request failed with %1%, retrying whole upload in 5s", http_code, + log::warning ) ; + retrying = true; + continue; + } + + if ( retrying ) + Log( "upload succeeded on retry", log::warning ); + Entry responseEntry = Entry( xml.Response() ); + AssignIDs( responseEntry ) ; + m_mtime = responseEntry.MTime(); + break; + } return true ; } diff --git a/libgrive/src/http/XmlResponse.cc b/libgrive/src/http/XmlResponse.cc index b25f1c4d..3df42f94 100644 --- a/libgrive/src/http/XmlResponse.cc +++ b/libgrive/src/http/XmlResponse.cc @@ -28,6 +28,11 @@ XmlResponse::XmlResponse() : m_tb( new xml::TreeBuilder ) { } +void XmlResponse::Clear() +{ + m_tb.reset(new xml::TreeBuilder); +} + std::size_t XmlResponse::Write( const char *data, std::size_t count ) { m_tb->ParseData( data, count ) ; diff --git a/libgrive/src/protocol/AuthAgent.cc b/libgrive/src/protocol/AuthAgent.cc index 6a329ceb..30a97223 100644 --- a/libgrive/src/protocol/AuthAgent.cc +++ b/libgrive/src/protocol/AuthAgent.cc @@ -21,8 +21,10 @@ #include "http/Error.hh" #include "http/Header.hh" +#include "http/XmlResponse.hh" #include "util/log/Log.hh" #include "util/OS.hh" +#include "util/File.hh" #include @@ -69,8 +71,22 @@ long AuthAgent::Put( Header auth = AppendHeader(hdr) ; long response ; - while ( CheckRetry( - response = m_agent->Put( url, file, dest, AppendHeader(hdr) ) ) ) ; + bool keepTrying = true; + while ( keepTrying ) { + response = m_agent->Put( url, file, dest, auth ); + keepTrying = CheckRetry( response ); + if ( keepTrying ) { + file->Seek( 0, SEEK_SET ); + XmlResponse *xmlResponse = dynamic_cast(dest); + if( xmlResponse ) + xmlResponse->Clear(); + } + } + + // On 410 Gone or 412 Precondition failed, recovery may be possible so don't + // throw an exception + if ( response == 410 || response == 412 ) + return response; return CheckHttpResponse(response, url, auth) ; } From 307c934ce49f2d51174c315c9e270ebe97e09982 Mon Sep 17 00:00:00 2001 From: Visa Putkinen Date: Mon, 25 Nov 2013 00:14:29 +0200 Subject: [PATCH 3/3] Retry upload on XML error instead of crashing Sometimes the Google Drive API sends malformed XML which crashes grive. This patch adds a simple try-catch to Resource::Upload that retries the upload if an XML exception is thrown from AuthAgent::Put. --- libgrive/src/drive/Resource.cc | 36 ++++++++++++++++++++++++++++------ 1 file changed, 30 insertions(+), 6 deletions(-) diff --git a/libgrive/src/drive/Resource.cc b/libgrive/src/drive/Resource.cc index fdb6d641..9a02940b 100644 --- a/libgrive/src/drive/Resource.cc +++ b/libgrive/src/drive/Resource.cc @@ -607,11 +607,23 @@ bool Resource::Upload( os::Sleep( 5 ); } - http::StringResponse str ; - if ( post ) - http->Post( link, meta, &str, hdr ) ; - else - http->Put( link, meta, &str, hdr ) ; + try { + http::StringResponse str ; + if ( post ) + http->Post( link, meta, &str, hdr ) ; + else + http->Put( link, meta, &str, hdr ) ; + } catch ( Error &e ) { + std::string const *info = boost::get_error_info(e); + if ( info && (*info == "XML_Parse") ) { + Log( "Error parsing pre-upload response XML, retrying whole upload in 5s", + log::warning ); + retrying = true; + continue; + } else { + throw e; + } + } http::Header uphdr ; uphdr.Add( "Expect:" ) ; @@ -622,7 +634,19 @@ bool Resource::Upload( http::XmlResponse xml ; long http_code = 0; - http_code = http->Put( uplink, &file, &xml, uphdr ) ; + try { + http_code = http->Put( uplink, &file, &xml, uphdr ) ; + } catch ( Error &e ) { + std::string const *info = boost::get_error_info(e); + if ( info && (*info == "XML_Parse") ) { + Log( "Error parsing response XML, retrying whole upload in 5s", + log::warning ); + retrying = true; + continue; + } else { + throw e; + } + } if ( http_code == 410 || http_code == 412 ) { Log( "request failed with %1%, retrying whole upload in 5s", http_code,