Skip to content

Commit

Permalink
implementing retry system for pull project
Browse files Browse the repository at this point in the history
  • Loading branch information
@VitorVieiraZ
VitorVieiraZ committed Nov 4, 2024
1 parent 68a245a commit c4fd385
Show file tree
Hide file tree
Showing 2 changed files with 82 additions and 16 deletions.
83 changes: 68 additions & 15 deletions core/merginapi.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -265,6 +265,16 @@ void MerginApi::downloadNextItem( const QString &projectFullName )

DownloadQueueItem item = transaction.downloadQueue.takeFirst();

QString itemInfo = QStringLiteral( "downloadNextItem : DownloadQueueItem(path: %1, size: %2, version: %3, range: %4-%5, diff: %6, temp: %7)" )
.arg( item.filePath )
.arg( item.size )
.arg( item.version )
.arg( item.rangeFrom )
.arg( item.rangeTo )
.arg( item.downloadDiff )
.arg( item.tempFileName );
qDebug() << "Processing item:" << itemInfo;

QUrl url( mApiRoot + QStringLiteral( "/v1/project/raw/" ) + projectFullName );
QUrlQuery query;
// Handles special chars in a filePath (e.g prevents to convert "+" sign into a space)
Expand All @@ -287,7 +297,8 @@ void MerginApi::downloadNextItem( const QString &projectFullName )
}

QNetworkReply *reply = mManager.get( request );
connect( reply, &QNetworkReply::finished, this, &MerginApi::downloadItemReplyFinished );
connect( reply, &QNetworkReply::finished, this, [this, item]() { downloadItemReplyFinished( item ); } );

transaction.replyPullItems.insert( reply );

CoreUtils::log( "pull " + projectFullName, QStringLiteral( "Requesting item: " ) + url.toString() +
Expand Down Expand Up @@ -373,28 +384,23 @@ QString MerginApi::getApiKey( const QString &serverName )
return "not-secret-key";
}

void MerginApi::downloadItemReplyFinished()
void MerginApi::downloadItemReplyFinished( DownloadQueueItem item )
{
QNetworkReply *r = qobject_cast<QNetworkReply *>( sender() );
Q_ASSERT( r );

QString projectFullName = r->request().attribute( static_cast<QNetworkRequest::Attribute>( AttrProjectFullName ) ).toString();
QString tempFileName = r->request().attribute( static_cast<QNetworkRequest::Attribute>( AttrTempFileName ) ).toString();

Q_ASSERT( mTransactionalStatus.contains( projectFullName ) );
TransactionStatus &transaction = mTransactionalStatus[projectFullName];
Q_ASSERT( transaction.replyPullItems.contains( r ) );

if ( r->error() == QNetworkReply::NoError )
{
QByteArray data = r->readAll();

CoreUtils::log( "pull " + projectFullName, QStringLiteral( "Downloaded item (%1 bytes)" ).arg( data.size() ) );

QString tempFolder = getTempProjectDir( projectFullName );
QString tempFilePath = tempFolder + "/" + tempFileName;
createPathIfNotExists( tempFilePath );

// save to a tmp file, assemble at the end
QFile file( tempFilePath );
if ( file.open( QIODevice::WriteOnly ) )
Expand All @@ -406,13 +412,10 @@ void MerginApi::downloadItemReplyFinished()
{
CoreUtils::log( "pull " + projectFullName, "Failed to open for writing: " + file.fileName() );
}

transaction.transferedSize += data.size();
emit syncProjectStatusChanged( projectFullName, transaction.transferedSize / transaction.totalSize );

transaction.replyPullItems.remove( r );
r->deleteLater();

if ( !transaction.downloadQueue.isEmpty() )
{
// one request finished, let's start another one
Expand All @@ -428,6 +431,42 @@ void MerginApi::downloadItemReplyFinished()
// no more requests to start, but there are pending requests - let's do nothing and wait
}
}
else if ( item.retryCount < transaction.MAX_RETRY_COUNT && isRetryableNetworkError( r ) )
{
item.retryCount++;
// Put the item back at the front for retry
transaction.downloadQueue.prepend( item );

CoreUtils::log( "pull " + projectFullName, QStringLiteral( "Retrying download (attempt %1 of 5)" ).arg( transaction.retryCount ) );

QString itemInfo = QStringLiteral( "DownloadQueueItem ( path: %1, size: %2, version: %3, range: %4-%5, diff: %6, temp: %7 )" )
.arg( item.filePath )
.arg( item.size )
.arg( item.version )
.arg( item.rangeFrom )
.arg( item.rangeTo )
.arg( item.downloadDiff )
.arg( item.tempFileName );
// qDebug() << "Retrying item:" << itemInfo;

qDebug() << "Current queue:";
for ( const DownloadQueueItem &queueItem : transaction.downloadQueue )
{
QString queueItemInfo = QStringLiteral( "DownloadQueueItem ( path: %1, size: %2, version: %3, range: %4-%5, diff: %6, temp: %7)" )
.arg( queueItem.filePath )
.arg( queueItem.size )
.arg( queueItem.version )
.arg( queueItem.rangeFrom )
.arg( queueItem.rangeTo )
.arg( queueItem.downloadDiff )
.arg( queueItem.tempFileName );
qDebug() << queueItemInfo;
}

downloadNextItem( projectFullName );
transaction.replyPullItems.remove( r );
r->deleteLater();
}
else
{
QString serverMsg = extractServerErrorMsg( r->readAll() );
Expand All @@ -439,15 +478,12 @@ void MerginApi::downloadItemReplyFinished()
serverMsg = r->errorString();
}
CoreUtils::log( "pull " + projectFullName, QStringLiteral( "FAILED - %1. %2" ).arg( r->errorString(), serverMsg ) );

transaction.replyPullItems.remove( r );
r->deleteLater();

if ( !transaction.pullItemsAborting )
{
// the first failed request will abort all the other pending requests too, and finish pull with error
abortPullItems( projectFullName );

// signal a networking error - we may retry
int httpCode = r->attribute( QNetworkRequest::HttpStatusCodeAttribute ).toInt();
emit networkErrorOccurred( serverMsg, QStringLiteral( "Mergin API error: downloadFile" ), httpCode, projectFullName );
Expand Down Expand Up @@ -3222,8 +3258,8 @@ ProjectDiff MerginApi::compareProjectFiles(
/*
for ( MerginFile file : oldServerFilesMap )
{
// R-D/L-D
// TODO: need to do anything?
// R-D/L-D
// TODO: need to do anything?
}
*/

Expand Down Expand Up @@ -3945,3 +3981,20 @@ DownloadQueueItem::DownloadQueueItem( const QString &fp, qint64 s, int v, qint64
tempFileName = CoreUtils::uuidWithoutBraces( QUuid::createUuid() );
}

bool MerginApi::isRetryableNetworkError( QNetworkReply *reply )
{
Q_ASSERT( reply );

QNetworkReply::NetworkError err = reply->error();
int httpCode = reply->attribute( QNetworkRequest::HttpStatusCodeAttribute ).toInt();

bool isRetryableError = ( err == QNetworkReply::TimeoutError ||
err == QNetworkReply::TemporaryNetworkFailureError ||
err == QNetworkReply::NetworkSessionFailedError ||
err == QNetworkReply::UnknownNetworkError );

bool isRetryableHttpCode = ( httpCode == 500 || httpCode == 502 ||
httpCode == 503 || httpCode == 504 );

return isRetryableError || isRetryableHttpCode;
}
15 changes: 14 additions & 1 deletion core/merginapi.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -109,6 +109,7 @@ struct DownloadQueueItem
qint64 rangeTo = -1; //!< what range of bytes to download (-1 if downloading the whole file)
bool downloadDiff = false; //!< whether to download just the diff between the previous version and the current one
QString tempFileName; //!< relative filename of the temporary file where the downloaded content will be stored
int retryCount = 0;
};


Expand Down Expand Up @@ -142,6 +143,8 @@ struct TransactionStatus
Pull
};

static const int MAX_RETRY_COUNT = 5; //!< maximum number of retry attempts for failed network requests

qreal totalSize = 0; //!< total size (in bytes) of files to be pushed or pulled
qint64 transferedSize = 0; //!< size (in bytes) of amount of data transferred so far
QString transactionUUID; //!< only for push. Initially dummy non-empty string, after server confirms a valid UUID, on finish/cancel it is empty
Expand All @@ -166,6 +169,9 @@ struct TransactionStatus
QList<MerginFile> pushQueue; //!< pending list of files to push (at the end of transaction it is empty)
QList<MerginFile> pushDiffFiles; //!< these are just diff files for push - we don't remove them when pushing chunks (needed for finalization)

// retry handling
int retryCount = 0; //!< current number of retry attempts for failed network requests

QString projectDir;
QByteArray projectMetadata; //!< metadata of the new project (not parsed)
bool firstTimeDownload = false; //!< only for update. whether this is first time to download the project (on failure we would also remove the project folder)
Expand Down Expand Up @@ -657,7 +663,7 @@ class MerginApi: public QObject

// Pull slots
void pullInfoReplyFinished();
void downloadItemReplyFinished();
void downloadItemReplyFinished( DownloadQueueItem item );
void cacheServerConfig();

// Push slots
Expand Down Expand Up @@ -785,6 +791,13 @@ class MerginApi: public QObject
//! Works only when login, password and token is set in UserAuth
void refreshAuthToken();

/**
* Checks if a network error should trigger a retry attempt.
* \param reply Network reply to check for retryable errors
* \returns True if the error should trigger a retry, false otherwise
*/
bool isRetryableNetworkError( QNetworkReply *reply );

QNetworkRequest getDefaultRequest( bool withAuth = true );

bool projectFileHasBeenUpdated( const ProjectDiff &diff );
Expand Down

1 comment on commit c4fd385

@inputapp-bot
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

iOS - version 24.11.692211 just submitted!

Please sign in to comment.