[Asterisk-code-review] media_cache: Don't lock when curl the remote file (asterisk[master])

[George Joseph]

George Joseph has submitted this change. ( https://gerrit.asterisk.org/c/asterisk/+/16233 )

Change subject: media_cache: Don't lock when curl the remote file
......................................................................

media_cache: Don't lock when curl the remote file

When playing a remote sound file, which is not in cache, first we need
to download it with ast_bucket_file_retrieve.

This can take a while if the remote host is slow. The current CURL
timeout is 180secs, so in extreme situations, it can take 3 minutes to
return.

Because ast_media_cache_retrieve has a lock on all function, while we
are waiting for the delayed download, Asterisk is not able to play any
more files, even the files already cached locally.

ASTERISK-29544 #close

Change-Id: I8d4142b463ae4a1d4c41bff2bf63324821567408
---
M main/media_cache.c
1 file changed, 25 insertions(+), 2 deletions(-)

Approvals:
  Kevin Harwell: Looks good to me, but someone else must approve
  George Joseph: Looks good to me, approved; Approved for Submit



diff --git a/main/media_cache.c b/main/media_cache.c
index 1899fb4..7e1cbe3 100644
--- a/main/media_cache.c
+++ b/main/media_cache.c
@@ -157,13 +157,15 @@
 	char *file_path, size_t len)
 {
 	struct ast_bucket_file *bucket_file;
+	struct ast_bucket_file *tmp_bucket_file;
 	char *ext;
-	SCOPED_AO2LOCK(media_lock, media_cache);
-
 	if (ast_strlen_zero(uri)) {
 		return -1;
 	}
 
+	ao2_lock(media_cache);
+	ast_debug(5, "Looking for media at local cache, file: %s\n", uri);
+
 	/* First, retrieve from the ao2 cache here. If we find a bucket_file
 	 * matching the requested URI, ask the appropriate backend if it is
 	 * stale. If not; return it.
@@ -179,6 +181,7 @@
 			ao2_ref(bucket_file, -1);
 
 			ast_debug(5, "Returning media at local file: %s\n", file_path);
+			ao2_unlock(media_cache);
 			return 0;
 		}
 
@@ -187,6 +190,10 @@
 		ast_bucket_file_delete(bucket_file);
 		ao2_ref(bucket_file, -1);
 	}
+	/* We unlock to retrieve the file, because it can take a long time;
+	 * and we don't want to lock access to cached files while waiting
+	 */
+	ao2_unlock(media_cache);
 
 	/* Either this is new or the resource is stale; do a full retrieve
 	 * from the appropriate bucket_file backend
@@ -197,6 +204,21 @@
 		return -1;
 	}
 
+	/* we lock again, before updating cache */
+	ao2_lock(media_cache);
+
+	/* We can have duplicated buckets here, we check if already exists
+	 * before saving
+	 */
+	tmp_bucket_file = ao2_find(media_cache, uri, OBJ_SEARCH_KEY | OBJ_NOLOCK);
+	if (tmp_bucket_file) {
+		ao2_ref(tmp_bucket_file, -1);
+		ast_bucket_file_delete(bucket_file);
+		ao2_ref(bucket_file, -1);
+		ao2_unlock(media_cache);
+		return 0;
+	}
+
 	/* We can manipulate the 'immutable' bucket_file here, as we haven't
 	 * let anyone know of its existence yet
 	 */
@@ -210,6 +232,7 @@
 	ao2_ref(bucket_file, -1);
 
 	ast_debug(5, "Returning media at local file: %s\n", file_path);
+	ao2_unlock(media_cache);
 
 	return 0;
 }

-- 
To view, visit https://gerrit.asterisk.org/c/asterisk/+/16233
To unsubscribe, or for help writing mail filters, visit https://gerrit.asterisk.org/settings

Gerrit-Project: asterisk
Gerrit-Branch: master
Gerrit-Change-Id: I8d4142b463ae4a1d4c41bff2bf63324821567408
Gerrit-Change-Number: 16233
Gerrit-PatchSet: 6
Gerrit-Owner: Andre Barbosa <andre.emanuel.barbosa at gmail.com>
Gerrit-Reviewer: Friendly Automation
Gerrit-Reviewer: George Joseph <gjoseph at digium.com>
Gerrit-Reviewer: Joshua Colp <jcolp at sangoma.com>
Gerrit-Reviewer: Kevin Harwell <kharwell at digium.com>
Gerrit-MessageType: merged
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20210820/0e5559c8/attachment.html>