From 89506b4162935e529b90f16afa059e36a27c18f0 Mon Sep 17 00:00:00 2001 From: Visa Putkinen Date: Sun, 24 Nov 2013 14:22:49 +0200 Subject: [PATCH 1/7] Add autogenerated stuff to .gitignore --- .gitignore | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/.gitignore b/.gitignore index da71e17..6fa3768 100644 --- a/.gitignore +++ b/.gitignore @@ -4,3 +4,13 @@ grive.kdev4 .project .cproject build/ +/CMakeCache.txt +CMakeFiles +moc_*.cxx* +bgrive/ui_MainWindow.h +Makefile +*.a +bgrive/bgrive +grive/grive +libgrive/btest +*.cmake From c15bfbe8bc93e4d59810a0c47225e98fd8269428 Mon Sep 17 00:00:00 2001 From: Visa Putkinen Date: Sun, 24 Nov 2013 14:25:01 +0200 Subject: [PATCH 2/7] Ignore only .grive and .grive_state --- libgrive/src/drive/State.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/libgrive/src/drive/State.cc b/libgrive/src/drive/State.cc index 2d1dc94..124adec 100644 --- a/libgrive/src/drive/State.cc +++ b/libgrive/src/drive/State.cc @@ -60,7 +60,7 @@ void State::FromLocal( const fs::path& p ) bool State::IsIgnore( const std::string& filename ) { - return filename[0] == '.' ; + return filename == ".grive" || filename == ".grive_state"; } void State::FromLocal( const fs::path& p, Resource* folder ) From 8b434dad144341cc2b49d95a7ab4b048ca2b3a0c Mon Sep 17 00:00:00 2001 From: Visa Putkinen Date: Sun, 24 Nov 2013 14:34:03 +0200 Subject: [PATCH 3/7] Reduce number of syscalls while scanning local files --- libgrive/src/drive/Resource.cc | 6 +++--- libgrive/src/drive/State.cc | 5 +++-- 2 files changed, 6 insertions(+), 5 deletions(-) diff --git a/libgrive/src/drive/Resource.cc b/libgrive/src/drive/Resource.cc index 4010cab..f285211 100644 --- a/libgrive/src/drive/Resource.cc +++ b/libgrive/src/drive/Resource.cc @@ -252,7 +252,7 @@ void Resource::FromRemoteFile( const Entry& remote, const DateTime& last_sync ) void Resource::FromLocal( const DateTime& last_sync ) { fs::path path = Path() ; - assert( fs::exists( path ) ) ; + //assert( fs::exists( path ) ) ; // root folder is always in sync if ( !IsRoot() ) @@ -270,8 +270,8 @@ void Resource::FromLocal( const DateTime& last_sync ) m_state = ( m_mtime > last_sync ? local_new : remote_deleted ) ; m_name = path.filename().string() ; - m_kind = fs::is_directory(path) ? "folder" : "file" ; - m_md5 = fs::is_directory(path) ? "" : crypt::MD5::Get( path ) ; + //m_kind = fs::is_directory(path) ? "folder" : "file" ; + m_md5 = IsFolder() ? "" : crypt::MD5::Get( path ) ; } assert( m_state != unknown ) ; diff --git a/libgrive/src/drive/State.cc b/libgrive/src/drive/State.cc index 124adec..6c2ba61 100644 --- a/libgrive/src/drive/State.cc +++ b/libgrive/src/drive/State.cc @@ -84,19 +84,20 @@ void State::FromLocal( const fs::path& p, Resource* folder ) else { + bool is_dir = fs::is_directory(i->path()); // if the Resource object of the child already exists, it should // have been so no need to do anything here Resource *c = folder->FindChild( fname ) ; if ( c == 0 ) { - c = new Resource( fname, fs::is_directory(i->path()) ? "folder" : "file" ) ; + c = new Resource( fname, is_dir ? "folder" : "file" ) ; folder->AddChild( c ) ; m_res.Insert( c ) ; } c->FromLocal( m_last_sync ) ; - if ( fs::is_directory( i->path() ) ) + if ( is_dir ) FromLocal( *i, c ) ; } } From 986ab4acc6f0274d8d2cf54e52e80e0ce775ddee Mon Sep 17 00:00:00 2001 From: Visa Putkinen Date: Sun, 24 Nov 2013 14:34:52 +0200 Subject: [PATCH 4/7] Log XML on parse failure --- libgrive/src/xml/TreeBuilder.cc | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/libgrive/src/xml/TreeBuilder.cc b/libgrive/src/xml/TreeBuilder.cc index c1ad385..9d57501 100644 --- a/libgrive/src/xml/TreeBuilder.cc +++ b/libgrive/src/xml/TreeBuilder.cc @@ -21,6 +21,7 @@ #include "Error.hh" #include "Node.hh" +#include "util/log/Log.hh" #include @@ -72,8 +73,10 @@ void TreeBuilder::ParseData( const char *data, std::size_t count, bool last ) { is_new = false ; - if ( ::XML_Parse( m_impl->psr, data, count, last ) == 0 ) + if ( ::XML_Parse( m_impl->psr, data, count, last ) == 0 ) { + Log("Error parsing XML: %1%", data, log::error); BOOST_THROW_EXCEPTION( Error() << ExpatApiError("XML_Parse") ); + } } Node TreeBuilder::Parse( const std::string& xml ) From 645bb2e7d408718b0b457add629d6d34843cbaef Mon Sep 17 00:00:00 2001 From: Visa Putkinen Date: Tue, 26 Nov 2013 23:00:42 +0200 Subject: [PATCH 5/7] 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 745f274..6a329ce 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 84785ec4731b7a87b844a2339c400ab2e2e9d0ae Mon Sep 17 00:00:00 2001 From: Visa Putkinen Date: Mon, 25 Nov 2013 00:07:27 +0200 Subject: [PATCH 6/7] 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 f285211..7a7a256 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 b25f1c4..3df42f9 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 6a329ce..30a9722 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 3775572f4650797e7eeab0ba23e27f1f15f01cd5 Mon Sep 17 00:00:00 2001 From: Visa Putkinen Date: Mon, 25 Nov 2013 00:14:29 +0200 Subject: [PATCH 7/7] 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 7a7a256..2da7859 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,