Retry failed song downloads instead of getting stuck

MusicServerClient functions now take an optional on_failed callback,
called when a request fails or returns invalid JSON. Before, a failed
request was silently dropped, so a failure while preloading a song left
Playlist.is_song_downloading set forever and no more songs were fetched.

Song fetching retries the step that failed (download, queue or info)
every song_fetch_retry_seconds, so the server and the game keep the same
playlist.

MainWindow.gd

@@ -140,6 +140,9 @@ var songs: Array= []
 var soundtracks: Array = []
 var current_player: Node
 
+## Seconds to wait before retrying a song download step that failed.
+var song_fetch_retry_seconds: float = 5.0
+
 # Called when the node enters the scene tree for the first time.
 func _ready() -> void:
 	var debug: bool = OS.has_feature("debug")
@@ -277,21 +280,30 @@ func play_next_song() -> void:
 func fetch_song() -> void:
 	print("fetch_song")
 	Playlist.is_song_downloading = true
-	var new_song_object: SongObject = SongObject.new()
+	_download_random_song(SongObject.new())
-	var fetch_song_done: Callable = func fetch_song_done(body: PackedByteArray) -> void:
+
-		print("fetch_song_done")
+# Fetching a song takes three requests. If one fails, only that step is retried, so
+# the server and the game keep the same songs in their playlists.
+func _download_random_song(new_song_object: SongObject) -> void:
+	var song_downloaded: Callable = func song_downloaded(body: PackedByteArray) -> void:
+		print("song_downloaded")
 		var sound: AudioStream = AudioStreamMP3.new()
 		sound.data = body
 		new_song_object.song = sound
+		_add_song_to_queue(new_song_object)
+	MusicServerClient.fetch_random_song(Settings.use_low_played_mode, song_downloaded,
+			_retry_later.bind(_download_random_song.bind(new_song_object)))
 
-		var added_to_que_done: Callable = func added_to_que_done() -> void:
+func _add_song_to_queue(new_song_object: SongObject) -> void:
-			print("added_to_que_done")
+	MusicServerClient.add_to_queue(_fetch_song_info.bind(new_song_object),
-			var fetch_song_info_done: Callable = func fetch_song_info_done(data_received: Dictionary) -> void:
+			_retry_later.bind(_add_song_to_queue.bind(new_song_object)))
-				print("fetch_song_info_done")
+
+func _fetch_song_info(new_song_object: SongObject) -> void:
+	var song_info_fetched: Callable = func song_info_fetched(data_received: Dictionary) -> void:
+		print("song_info_fetched")
 		new_song_object.song_number = data_received.SongNo
 		new_song_object.soundtrack_title = data_received.Soundtrack
 		new_song_object.song_title = data_received.Song
-			
 		print("new_song_object: ", new_song_object.to_string())
 		Playlist.add_to_playlist(new_song_object)
 		next_button.disabled = false
@@ -301,11 +313,12 @@ func fetch_song() -> void:
 			Playlist.is_song_downloading = false
 		if Settings.is_debug:
 			update_song_list()
-			MusicServerClient.fetch_song_info(fetch_song_info_done)
+	MusicServerClient.fetch_song_info(song_info_fetched,
+			_retry_later.bind(_fetch_song_info.bind(new_song_object)))
 
-		MusicServerClient.add_to_queue(added_to_que_done)
+func _retry_later(step: Callable) -> void:
-
+	print("Song download failed, retrying in %s seconds" % song_fetch_retry_seconds)
-	MusicServerClient.fetch_random_song(Settings.use_low_played_mode, fetch_song_done)
+	get_tree().create_timer(song_fetch_retry_seconds).timeout.connect(step)
 
 func update_song_list() -> void:
 	print("update_song_list")

music_server_client.gd

@@ -17,108 +17,123 @@ func _ready() -> void:
 
 ## Fetches the songs already in the server playlist, as an Array of
 ## [code]{SongNo, Soundtrack, Song}[/code] dictionaries.
-func fetch_song_list(on_done: Callable) -> void:
+func fetch_song_list(on_done: Callable, on_failed: Callable = Callable()) -> void:
-    get_json("/music/list", on_done)
+    get_json("/music/list", on_done, on_failed)
 
 
 ## Downloads a random song as MP3 bytes. With [param low_played] the server prefers
 ## soundtracks that have been played the least.
-func fetch_random_song(low_played: bool, on_done: Callable) -> void:
+func fetch_random_song(
-    get_bytes("/music/rand/low" if low_played else "/music/rand", on_done)
+        low_played: bool, on_done: Callable, on_failed: Callable = Callable()) -> void:
+    get_bytes("/music/rand/low" if low_played else "/music/rand", on_done, on_failed)
 
 
 ## Adds the most recently fetched random song to the server playlist.
-func add_to_queue(on_done: Callable) -> void:
+func add_to_queue(on_done: Callable, on_failed: Callable = Callable()) -> void:
-    send("/music/addQue", on_done)
+    send("/music/addQue", on_done, on_failed)
 
 
 ## Fetches [code]{SongNo, Soundtrack, Song}[/code] for the most recently queued song.
-func fetch_song_info(on_done: Callable) -> void:
+func fetch_song_info(on_done: Callable, on_failed: Callable = Callable()) -> void:
-    get_json("/music/info", on_done)
+    get_json("/music/info", on_done, on_failed)
 
 
 ## Downloads a song from the server playlist as MP3 bytes.
-func fetch_song(song_no: int, on_done: Callable) -> void:
+func fetch_song(song_no: int, on_done: Callable, on_failed: Callable = Callable()) -> void:
-    get_bytes("/music?song=" + str(song_no), on_done)
+    get_bytes("/music?song=" + str(song_no), on_done, on_failed)
 
 
 ## Downloads the sound test song as MP3 bytes.
-func fetch_sound_test_song(on_done: Callable) -> void:
+func fetch_sound_test_song(on_done: Callable, on_failed: Callable = Callable()) -> void:
-    get_bytes("/music/soundTest", on_done)
+    get_bytes("/music/soundTest", on_done, on_failed)
 
 
 ## Clears the server playlist.
-func reset_playlist(on_done: Callable) -> void:
+func reset_playlist(on_done: Callable, on_failed: Callable = Callable()) -> void:
-    send("/music/reset", on_done)
+    send("/music/reset", on_done, on_failed)
 
 
 ## Fetches every soundtrack name as an Array of Strings.
-func fetch_all_soundtracks(on_done: Callable) -> void:
+func fetch_all_soundtracks(on_done: Callable, on_failed: Callable = Callable()) -> void:
-    get_json("/music/all", on_done)
+    get_json("/music/all", on_done, on_failed)
 
 
 ## Fetches every soundtrack name, sorted, as an Array of Strings.
-func fetch_all_soundtracks_ordered(on_done: Callable) -> void:
+func fetch_all_soundtracks_ordered(on_done: Callable, on_failed: Callable = Callable()) -> void:
-    get_json("/music/all/order", on_done)
+    get_json("/music/all/order", on_done, on_failed)
 
 
 ## Records in the server statistics that a song has been played.
 func mark_song_played(song_no: int) -> void:
-    send("/music/played?song=" + str(song_no), Callable(), HTTPClient.METHOD_PUT)
+    send("/music/played?song=" + str(song_no), Callable(), Callable(), HTTPClient.METHOD_PUT)
 
 
 ## Fetches the names of all character images.
-func fetch_character_list(on_done: Callable) -> void:
+func fetch_character_list(on_done: Callable, on_failed: Callable = Callable()) -> void:
-    get_json("/characters", on_done)
+    get_json("/characters", on_done, on_failed)
 
 
 ## Downloads a character image as PNG bytes.
-func fetch_character_image(character_name: String, on_done: Callable) -> void:
+func fetch_character_image(
-    get_bytes("/character?name=" + character_name.uri_encode(), on_done)
+        character_name: String, on_done: Callable, on_failed: Callable = Callable()) -> void:
+    get_bytes("/character?name=" + character_name.uri_encode(), on_done, on_failed)
 
 
 ## Starts syncing the server's soundtrack library with its music folder.
-func start_sync(on_done: Callable) -> void:
+func start_sync(on_done: Callable, on_failed: Callable = Callable()) -> void:
-    send("/sync", on_done)
+    send("/sync", on_done, on_failed)
 
 
 ## Fetches the sync state: [code]{progress, time_spent}[/code] while running, or the
 ## sync result once finished.
-func fetch_sync_progress(on_done: Callable) -> void:
+func fetch_sync_progress(on_done: Callable, on_failed: Callable = Callable()) -> void:
-    get_json("/sync/progress", on_done)
+    get_json("/sync/progress", on_done, on_failed)
 
 
 ## Requests [param path] and calls [param on_done] with the parsed JSON.
-## Nothing is called if the request fails or the body is not valid JSON.
+## [param on_failed] is called with no arguments if the request fails or the body is
-func get_json(path: String, on_done: Callable) -> void:
+## not valid JSON.
+func get_json(path: String, on_done: Callable, on_failed: Callable = Callable()) -> void:
     _request(path, HTTPClient.METHOD_GET, func(body: PackedByteArray) -> void:
         var json: JSON = JSON.new()
         if json.parse(body.get_string_from_utf8()) != OK:
             push_warning("Invalid JSON from %s: %s" % [path, json.get_error_message()])
+            _call_if_valid(on_failed)
             return
         on_done.call(json.data)
-    )
+    , on_failed)
 
 
-## Requests [param path] and calls [param on_done] with the raw response body.
+## Requests [param path] and calls [param on_done] with the raw response body, or
-func get_bytes(path: String, on_done: Callable) -> void:
+## [param on_failed] with no arguments if the request fails.
-    _request(path, HTTPClient.METHOD_GET, on_done)
+func get_bytes(path: String, on_done: Callable, on_failed: Callable = Callable()) -> void:
+    _request(path, HTTPClient.METHOD_GET, on_done, on_failed)
 
 
-## Requests [param path] and calls [param on_done] with no arguments when it succeeds.
+## Requests [param path] and calls [param on_done] with no arguments when it succeeds,
-## [param on_done] may be an empty Callable when the result does not matter.
+## or [param on_failed] when it fails. Either may be an empty Callable.
-func send(path: String, on_done: Callable, method: int = HTTPClient.METHOD_GET) -> void:
+func send(
+        path: String,
+        on_done: Callable,
+        on_failed: Callable = Callable(),
+        method: int = HTTPClient.METHOD_GET,
+) -> void:
     _request(path, method, func(_body: PackedByteArray) -> void:
-        if on_done.is_valid():
+        _call_if_valid(on_done)
-            on_done.call()
+    , on_failed)
-    )
 
 
-func _request(path: String, method: int, on_success: Callable) -> void:
+func _request(path: String, method: int, on_success: Callable, on_failed: Callable) -> void:
     var url: String = Settings.default_path + path
     transport.request(url, method, func(response_code: int, body: PackedByteArray) -> void:
         if response_code != 200:
             push_warning("Request to %s failed with response code %d" % [url, response_code])
+            _call_if_valid(on_failed)
             return
         on_success.call(body)
     )
+
+
+static func _call_if_valid(callback: Callable) -> void:
+    if callback.is_valid():
+        callback.call()

tests/integration/test_main_window.gd

@@ -78,3 +78,44 @@ func test_cannot_add_player_without_name() -> void:
     _add_player("")
 
     assert_array(Settings.player_array).is_empty()
+
+
+func test_failed_song_download_is_retried_until_the_server_answers() -> void:
+    _window.song_fetch_retry_seconds = 0.05
+    var songs_before: int = Playlist.song_object_array.size()
+    var mp3: PackedByteArray = FileAccess.get_file_as_bytes("res://tests/data/short.mp3")
+    _server.respond_bytes("/music/rand", PackedByteArray(), 503)
+    var rand_requests_before: int = _server.count("/music/rand")
+
+    _runner.invoke("fetch_song")
+    await await_millis(200)
+
+    assert_int(_server.count("/music/rand") - rand_requests_before).is_greater(1)
+    assert_int(Playlist.song_object_array.size()).is_equal(songs_before)
+    assert_bool(Playlist.is_song_downloading).is_true()
+
+    _server.respond_bytes("/music/rand", mp3)
+    await await_millis(200)
+
+    assert_int(Playlist.song_object_array.size()).is_equal(songs_before + 1)
+    assert_bool(Playlist.is_song_downloading).is_false()
+
+
+func test_failed_song_info_retries_only_that_step() -> void:
+    _window.song_fetch_retry_seconds = 0.05
+    var songs_before: int = Playlist.song_object_array.size()
+    _server.respond_bytes("/music/info", PackedByteArray(), 503)
+    var rand_before: int = _server.count("/music/rand")
+    var queue_before: int = _server.count("/music/addQue")
+
+    _runner.invoke("fetch_song")
+    await await_millis(200)
+    _next_song_is(SONG_INFO[1])
+    await await_millis(200)
+
+    # The song was downloaded and queued once; only the info request was repeated
+    assert_int(_server.count("/music/rand") - rand_before).is_equal(1)
+    assert_int(_server.count("/music/addQue") - queue_before).is_equal(1)
+    assert_int(Playlist.song_object_array.size()).is_equal(songs_before + 1)
+    assert_str(Playlist.song_object_array.back().song_title).is_equal("Dancing Mad")
+    assert_bool(Playlist.is_song_downloading).is_false()

tests/unit/test_music_server_client.gd

@@ -63,6 +63,39 @@ func test_invalid_json_does_not_call_back() -> void:
     assert_bool(called[0]).is_false()
 
 
+func test_failed_request_calls_on_failed() -> void:
+    _server.respond_bytes("/music/rand", PackedByteArray(), 503)
+    var outcome: Array[String] = []
+
+    MusicServerClient.fetch_random_song(false,
+            func(_body: PackedByteArray) -> void: outcome.append("done"),
+            func() -> void: outcome.append("failed"))
+
+    assert_array(outcome).contains_exactly(["failed"])
+
+
+func test_invalid_json_calls_on_failed() -> void:
+    _server.respond_bytes("/music/info", "not json".to_utf8_buffer())
+    var outcome: Array[String] = []
+
+    MusicServerClient.fetch_song_info(
+            func(_data: Variant) -> void: outcome.append("done"),
+            func() -> void: outcome.append("failed"))
+
+    assert_array(outcome).contains_exactly(["failed"])
+
+
+func test_successful_request_does_not_call_on_failed() -> void:
+    _server.respond_bytes("/music/addQue", PackedByteArray())
+    var outcome: Array[String] = []
+
+    MusicServerClient.add_to_queue(
+            func() -> void: outcome.append("done"),
+            func() -> void: outcome.append("failed"))
+
+    assert_array(outcome).contains_exactly(["done"])
+
+
 func test_send_calls_back_without_arguments() -> void:
     _server.respond_bytes("/music/reset", PackedByteArray())
     var called: Array[bool] = [false]