Infinite loop in elm_map
Open, Showstopper IssuesPublic
Actions

Description

In the "ecore_file_download_full" function of the "src/lib/ecore_file/ecore_file_download.c" file, if the destination file exists the function returns false.
In the "_download_job" function of the "src/lib/elementary/elm_map.c" file, if the destination file exists the function returns an error, but does not remove the file from the list.

Here are the two messages I have to infinity.
[24/10/2018 10:45:27] [Error] ecore_file:lib/ecore_file/ecore_file_download.c:214:ecore_file_download_full(): /home/ordissimo/.cache/elm_map/177078273/15/16595/11278.png already exists
[24/10/2018 10:45:27] [Error] elementary:lib/elementary/elm_map.c:815:_download_job(): Can't start to download from http://a.tile.openstreetmap.org/15/16595/11278.png to /home/ordissimo/.cache/elm_map/177078273/15/16595/11278.png

The file is in the cache, the ecore_file_download_full function should not return false but true. If the file does not match, it must be re-downloaded.
The ecore_file_download_full function must return an error code to indicate how the calling function should behave.

Details

Differential Revisions: D7617: elm_map: Make more robust elm_map

Related Objects

Mentioned Here: rEFL02d352e1f29c: ecore_file_download: use new efl_net_dialer_http.

thierry1970 created this task.Oct 29 2018, 9:51 AM

Herald added a subscriber: devilhorns. · View Herald TranscriptOct 29 2018, 9:51 AM

thierry1970 renamed this task from Endlosschleife in elm_map to Infinite loop in elm_map.Oct 29 2018, 10:28 AM

thierry1970 edited projects, added efl (efl-1.22); removed efl.Oct 29 2018, 10:30 AM

@barbieri this is from your commit: 02d352e1f29

please have a look.

That does not solve my problem.
Here is the patch I apply to solve the problem:

--- a/src/lib/ecore_file/ecore_file_download.c
+++ b/src/lib/ecore_file/ecore_file_download.c
@@ -211,8 +211,7 @@ ecore_file_download_full(const char *url
    free(dir);
    if (ecore_file_exists(dst))
      {
-        ERR("%s already exists", dst);
-        return EINA_FALSE;
+        return EINA_TRUE;
      }
-
    loop = efl_main_loop_get();

I do not push it because it is not the right solution. Currently, in Elm_Map, the failure causes a new download of the file. The failure can have multiple causes and it is not treated.

In T7443#125791, @thierry1970 wrote:

That does not solve my problem.

de: habe ich auch nicht behauptet. ich habe nur demjenigen der den commit eingeflegt hat bescheid gesagt.

en: i never claimed that. i just asked the committer to have a look at it.

@thierry1970 dein fix sieht alles in allem nicht ganz falsch aus, funktioniert die map denn dann ?

Yes perfectly.
It should be checked if the cache file is not corrupted.
We should know why the download was not done and let the evolved objects decide what to do.

hi guys, I can't easily check this... but if the patch by @thierry1970 fixes (return true), I'd rather suggest the caller to simply unlink(file) before trying again.

Bad idea, it's like removing the cache.

This is what should be done and repetcute on all objects qyu uses the function.

diff --git a/src/lib/ecore_file/Ecore_File.h b/src/lib/ecore_file/Ecore_File.h
index 23a0521945..10391af656 100644
--- a/src/lib/ecore_file/Ecore_File.h
+++ b/src/lib/ecore_file/Ecore_File.h
@@ -78,6 +78,16 @@ typedef enum _Ecore_File_Event
    ECORE_FILE_EVENT_CLOSED             /**< Closed file event */
 } Ecore_File_Event;
 
+typedef enum _Ecore_File_Download_Error
+{
+   ECORE_FILE_DOWNLOAD_DONE,
+   ECORE_FILE_DOWNLOAD_NO_DIRECTORY_EXIST,
+   ECORE_FILE_DOWNLOAD_FILE_EXIST,
+   ECORE_FILE_DOWNLOAD_NO_URL,
+   ECORE_FILE_DOWNLOAD_NOT_URL,
+   ECORE_FILE_DOWNLOAD_ERROR
+} Ecore_File_Download_Error;
+
 /**
  * @typedef Ecore_File_Monitor_Cb
  * Callback type used when a monitored directory has changes.
diff --git a/src/lib/ecore_file/ecore_file_download.c b/src/lib/ecore_file/ecore_file_download.c
index 54666693d2..e7dd65753e 100644
--- a/src/lib/ecore_file/ecore_file_download.c
+++ b/src/lib/ecore_file/ecore_file_download.c
@@ -174,7 +174,7 @@ _ecore_file_download_headers_foreach_cb(const Eina_Hash *hash EINA_UNUSED, const
    return EINA_TRUE;
 }
 
-EAPI Eina_Bool
+EAPI Ecore_File_Download_Error
 ecore_file_download_full(const char *url,
                          const char *dst,
                          Ecore_File_Download_Completion_Cb completion_cb,
@@ -192,13 +192,13 @@ ecore_file_download_full(const char *url,
    if (!url)
      {
         CRI("Download URL is null");
-        return EINA_FALSE;
+        return ECORE_FILE_DOWNLOAD_NO_URL;
      }
 
    if (!strstr(url, "://"))
      {
         ERR("'%s' is not an URL, missing protocol://", url);
-        return EINA_FALSE;
+        return ECORE_FILE_DOWNLOAD_NOT_URL;
      }
 
    dir = ecore_file_dir_get(dst);
@@ -206,20 +206,20 @@ ecore_file_download_full(const char *url,
      {
         ERR("%s is not a directory", dir);
         free(dir);
-        return EINA_FALSE;
+        return ECORE_FILE_DOWNLOAD_NO_DIRECTORY_EXIST;
      }
    free(dir);
    if (ecore_file_exists(dst))
      {
         ERR("%s already exists", dst);
-        return EINA_FALSE;
+        return ECORE_FILE_DOWNLOAD_FILE_EXIST;
      }
 
    loop = efl_main_loop_get();
-   EINA_SAFETY_ON_NULL_RETURN_VAL(loop, EINA_FALSE);
+   EINA_SAFETY_ON_NULL_RETURN_VAL(loop, ECORE_FILE_DOWNLOAD_ERROR);
 
    job = calloc(1, sizeof(Ecore_File_Download_Job));
-   EINA_SAFETY_ON_NULL_RETURN_VAL(job, EINA_FALSE);
+   EINA_SAFETY_ON_NULL_RETURN_VAL(job, ECORE_FILE_DOWNLOAD_ERROR);
    ECORE_MAGIC_SET(job, ECORE_MAGIC_FILE_DOWNLOAD_JOB);
 
    job->input = efl_add(EFL_NET_DIALER_HTTP_CLASS, loop,
@@ -258,11 +258,11 @@ ecore_file_download_full(const char *url,
      }
 
    if (job_ret) *job_ret = job;
-   return EINA_TRUE;
+   return ECORE_FILE_DOWNLOAD_DONE;
 
  error_dial:
    efl_del(job->copier);
-   return EINA_FALSE; /* copier's "del" event will delete everything else */
+   return ECORE_FILE_DOWNLOAD_ERROR; /* copier's "del" event will delete everything else */
 
  error_copier:
    efl_del(job->output);
@@ -271,7 +271,7 @@ ecore_file_download_full(const char *url,
  error_input:
    ECORE_MAGIC_SET(job, ECORE_MAGIC_NONE);
    free(job);
-   return EINA_FALSE;
+   return ECORE_FILE_DOWNLOAD_ERROR;
 }
 
 EAPI Eina_Bool

hi guys, I can't easily check this... but if the patch by @thierry1970 fixes (return true), I'd rather suggest the caller to simply unlink(file) before trying again.

In addition it is not feasible since I do not know why it failed.

it's not feasible to change the ABI from a boolean to an enum... and that's the legacy API, not the new efl.net, AFAIU that one returns an errno, which allows you to do that.

unlink and try again is the most feasible solution, since there is nothing else you can do from a program...

btw, you can check the stat() post fact, with that do an unlink... or show an error to the user (ie: dir doesn't exist)

I agree. It is for this reason that I did not propose my changes.
Ok for the directory. What generates the infinite loop is the return FALSE, without any error except the presence of the file in the cache.

The cache is used for optimization in this fix is the right solution?

--- a/src/lib/ecore_file/ecore_file_download.c
+++ b/src/lib/ecore_file/ecore_file_download.c
@@ -211,8 +211,7 @@ ecore_file_download_full(const char *url
    free(dir);
    if (ecore_file_exists(dst))
      {
-        ERR("%s already exists", dst);
-        return EINA_FALSE;
+        return EINA_TRUE;
      }
-
    loop = efl_main_loop_get();

i don't think it's correct, since if it's there, the caller can check prior to asking for a file download.

if (!exists) {
    download()
}

The handling must be done elsewhere

for instance, if it's an image, you can try to load it if exists, if it loads correctly, then use that, otherwise unlink + re-download

michael.bouchaud added a subscriber: michael.bouchaud.Jan 12 2019, 2:11 AM

michael.bouchaud added a revision: D7617: elm_map: Make more robust elm_map.Jan 14 2019, 4:35 AM

I do as gustavo suggested, if the file already exists just try to load it. If it's an invaldid tile file (image) re-download it.

Infinite loop in elm_mapOpen, Showstopper IssuesPublicActions

Description

Details

Related Objects

Infinite loop in elm_map
Open, Showstopper IssuesPublic
Actions