Fix for lp:785203 (PHP client issue).

ChangeLog

@@ -3,6 +3,9 @@
   * gearman_execute() uses the function parameter for the reducer, not the mapper/split function.
   * Additional documentation update.
 
+
+  * Issue with PHP client using automated task free (Thanks rosmiitto!)
+
 0.21 Wed Jun 15 21:01:04 PDT 2011
   * Support for "drop function" in admin client
   * Fixed issue where server would not report on a bad unique value due to size

libgearman/add.cc

@@ -327,6 +327,7 @@ gearman_task_st *add_reducer_task(gearman_client_st *client,
     gearman_task_free(task);
     task= NULL;
   }
+  task->type= GEARMAN_TASK_KIND_EXECUTE;
 
   return task;
 }

libgearman/client.cc

@@ -511,13 +511,15 @@ gearman_return_t gearman_client_job_status(gearman_client_st *client,
 {
   gearman_return_t ret;
 
-  gearman_task_st do_task, *do_task_ptr;
-  do_task_ptr= gearman_client_add_task_status(client, &do_task, client,
-                                              job_handle, &ret);
+  gearman_task_st do_task;
+  gearman_task_st *do_task_ptr= gearman_client_add_task_status(client, &do_task, client,
+                                                               job_handle, &ret);
   if (gearman_failed(ret))
+  {
     return ret;
-
+  }
   assert(do_task_ptr);
+  do_task_ptr->type= GEARMAN_TASK_KIND_DO;
 
   gearman_task_clear_fn(do_task_ptr);
 
@@ -1178,7 +1180,6 @@ static void *_client_do(gearman_client_st *client, gearman_command_t command,
                         const void *workload_str, size_t workload_size,
                         size_t *result_size, gearman_return_t *ret_ptr)
 {
-  gearman_task_st do_task, *do_task_ptr;
   gearman_string_t function= { gearman_string_param_cstr(function_name) };
   gearman_unique_t local_unique= gearman_unique_make(unique, unique ? strlen(unique) : 0);
   gearman_string_t workload= { static_cast<const char*>(workload_str), workload_size };
@@ -1189,22 +1190,30 @@ static void *_client_do(gearman_client_st *client, gearman_command_t command,
 
   if (not client)
   {
-    *ret_ptr= GEARMAN_ERRNO;
+    *ret_ptr= GEARMAN_INVALID_ARGUMENT;
     errno= EINVAL;
     return NULL;
   }
 
-  do_task_ptr= add_task(client, &do_task, NULL, command,
-                        function,
-                        local_unique,
-                        workload,
-                        time_t(0),
-                        gearman_actions_do_default());
+  if (not function_name)
+  {
+    *ret_ptr= GEARMAN_INVALID_ARGUMENT;
+    return NULL;
+  }
+
+  gearman_task_st do_task;
+  gearman_task_st *do_task_ptr= add_task(client, &do_task, NULL, command,
+                                         function,
+                                         local_unique,
+                                         workload,
+                                         time_t(0),
+                                         gearman_actions_do_default());
   if (not do_task_ptr)
   {
     *ret_ptr= gearman_universal_error_code(client->universal);
     return NULL;
   }
+  do_task_ptr->type= GEARMAN_TASK_KIND_DO;
 
   gearman_return_t ret= gearman_client_run_tasks(client);
 
@@ -1220,7 +1229,6 @@ static void *_client_do(gearman_client_st *client, gearman_command_t command,
   else if (gearman_success(ret) and do_task_ptr->result_rc == GEARMAN_SUCCESS)
   {
     *ret_ptr= do_task_ptr->result_rc;
-    assert(do_task_ptr);
     if (do_task_ptr->result_ptr)
     {
       gearman_string_t result= gearman_result_take_string(do_task_ptr->result_ptr);
@@ -1255,6 +1263,17 @@ static gearman_return_t _client_do_background(gearman_client_st *client,
                                               gearman_string_t &workload,
                                               gearman_job_handle_t job_handle)
 {
+  if (not client)
+  {
+    errno= EINVAL;
+    return GEARMAN_INVALID_ARGUMENT;
+  }
+
+  if (gearman_size(function) == 0)
+  {
+    return gearman_error(client->universal, GEARMAN_INVALID_ARGUMENT, "function arguement was empty");
+  }
+
   gearman_task_st do_task, *do_task_ptr;
   do_task_ptr= add_task(client, &do_task, 
                         client, 
@@ -1268,19 +1287,24 @@ static gearman_return_t _client_do_background(gearman_client_st *client,
   {
     return gearman_universal_error_code(client->universal);
   }
+  do_task_ptr->type= GEARMAN_TASK_KIND_DO;
 
-  gearman_task_clear_fn(do_task_ptr);
+  gearman_return_t ret;
+  do {
+    ret= gearman_client_run_tasks(client);
+    
+    // If either of the following is ever true, we will end up in an
+    // infinite loop
+    assert(ret != GEARMAN_IN_PROGRESS and ret != GEARMAN_JOB_EXISTS);
 
-  gearman_return_t ret= gearman_client_run_tasks(client);
-  if (ret != GEARMAN_IO_WAIT)
+  } while (gearman_continue(ret));
+
+  if (job_handle)
   {
-    if (job_handle)
-    {
-      strncpy(job_handle, do_task.job_handle, GEARMAN_JOB_HANDLE_SIZE);
-    }
-    client->new_tasks= 0;
-    client->running_tasks= 0;
+    strncpy(job_handle, do_task.job_handle, GEARMAN_JOB_HANDLE_SIZE);
   }
+  client->new_tasks= 0;
+  client->running_tasks= 0;
   gearman_task_free(&do_task);
 
   return ret;

libgearman/execute.cc

@@ -159,6 +159,7 @@ gearman_task_st *gearman_execute(gearman_client_st *client,
     return NULL;
   }
 
+  task->type= GEARMAN_TASK_KIND_EXECUTE;
   gearman_client_run_tasks(client);
 
   return task;

libgearman/run.cc

@@ -337,7 +337,7 @@ gearman_return_t _client_run_task(gearman_client_st *client, gearman_task_st *ta
   client->running_tasks--;
   task->state= GEARMAN_TASK_STATE_FINISHED;
 
-  if (client->options.free_tasks)
+  if (client->options.free_tasks and task->type == GEARMAN_TASK_KIND_ADD_TASK)
   {
     gearman_task_free(task);
   }

libgearman/task.cc

@@ -79,6 +79,8 @@ gearman_task_st *gearman_task_internal_create(gearman_client_st *client, gearman
   task->options.is_running= false;
   task->options.was_reduced= false;
 
+  task->type= GEARMAN_TASK_KIND_ADD_TASK;
+
   task->state= GEARMAN_TASK_STATE_NEW;
   task->created_id= 0;
   task->numerator= 0;

libgearman/task.h

@@ -78,6 +78,12 @@ enum gearman_task_state_t {
   GEARMAN_TASK_STATE_FINISHED
 };
 
+enum gearman_task_kind_t {
+  GEARMAN_TASK_KIND_ADD_TASK,
+  GEARMAN_TASK_KIND_EXECUTE,
+  GEARMAN_TASK_KIND_DO
+};
+
 struct gearman_task_st
 {
   struct {
@@ -87,6 +93,7 @@ struct gearman_task_st
     bool is_running;
     bool was_reduced;
   } options;
+  enum gearman_task_kind_t type;
   enum gearman_task_state_t state;
   uint32_t created_id;
   uint32_t numerator;

tests/client_test.cc

@@ -456,7 +456,9 @@ static test_return_t submit_job_test(void *object)
   gearman_return_t rc;
   void *job_result= gearman_client_do(client, worker_function, NULL, gearman_string_param(value), &result_length, &rc);
 
-  test_compare_got(GEARMAN_SUCCESS, rc, gearman_client_error(client) ? gearman_client_error(client) : gearman_strerror(rc));
+  test_compare_got(GEARMAN_SUCCESS, 
+                   rc, gearman_client_error(client) ? gearman_client_error(client) : gearman_strerror(rc));
+
   test_truth(job_result);
   test_compare(gearman_size(value), result_length);
 
@@ -758,6 +760,88 @@ static test_return_t loop_test(void *)
   return TEST_SUCCESS;
 }
 
+static test_return_t regression_785203_do_test(void *object)
+{
+  gearman_client_st *original_client= (gearman_client_st *)object;
+  test_true(original_client);
+
+  const char *worker_function= (const char *)gearman_client_context(original_client);
+  test_truth(original_client);
+
+  gearman_client_st *client;
+  test_truth(client= gearman_client_create(NULL));
+
+  test_compare(GEARMAN_SUCCESS,
+               gearman_client_add_server(client, NULL, CLIENT_TEST_PORT));
+
+  gearman_client_add_options(client, GEARMAN_CLIENT_FREE_TASKS);
+  { // All defaults, except timeout_return
+    test_truth(client->options.allocated);
+    test_false(client->options.non_blocking);
+    test_false(client->options.unbuffered_result);
+    test_false(client->options.no_new);
+    test_truth(client->options.free_tasks);
+  }
+
+  gearman_return_t rc;
+  size_t result_length;
+  void *result= gearman_client_do(client, worker_function, NULL, 
+                                  gearman_literal_param("keep it rocking and sing"),
+                                  &result_length, &rc);
+  test_true(result);
+  free(result);
+
+  gearman_client_free(client);
+
+  return TEST_SUCCESS;
+}
+
+static test_return_t regression_785203_do_background_test(void *object)
+{
+  gearman_client_st *original_client= (gearman_client_st *)object;
+  test_true(original_client);
+
+  const char *worker_function= (const char *)gearman_client_context(original_client);
+  test_truth(worker_function);
+
+  gearman_client_st *client;
+  test_truth(client= gearman_client_create(NULL));
+
+  test_compare(GEARMAN_SUCCESS,
+               gearman_client_add_server(client, NULL, CLIENT_TEST_PORT));
+
+  gearman_client_add_options(client, GEARMAN_CLIENT_FREE_TASKS);
+  { // All defaults, except timeout_return
+    test_truth(client->options.allocated);
+    test_false(client->options.non_blocking);
+    test_false(client->options.unbuffered_result);
+    test_false(client->options.no_new);
+    test_truth(client->options.free_tasks);
+  }
+
+  gearman_job_handle_t job_handle;
+  test_compare_got(GEARMAN_SUCCESS,
+                   gearman_client_do_background(client, worker_function, 
+                                                NULL,  // No unique requested
+                                                gearman_literal_param("keep it rocking and sing"),
+                                                job_handle), 
+                   gearman_client_error(client));
+
+  gearman_return_t ret;
+  do
+  {
+    uint32_t numerator;
+    uint32_t denominator;
+
+    ret= gearman_client_job_status(client, job_handle, NULL, NULL, &numerator, &denominator);
+  } while (gearman_continue(ret));
+  test_compare(GEARMAN_SUCCESS, ret);
+
+  gearman_client_free(client);
+
+  return TEST_SUCCESS;
+}
+
 static test_return_t regression_768317_test(void *object)
 {
   gearman_client_st *client= (gearman_client_st *)object;
@@ -901,6 +985,24 @@ static test_return_t pre_chunk(void *object)
   return TEST_SUCCESS;
 }
 
+static test_return_t pre_free_tasks(void *object)
+{
+  client_test_st *all= (client_test_st *)object;
+
+  gearman_client_add_options(all->client(), GEARMAN_CLIENT_FREE_TASKS);
+
+  return TEST_SUCCESS;
+}
+
+static test_return_t post_free_tasks(void *object)
+{
+  client_test_st *all= (client_test_st *)object;
+
+  gearman_client_remove_options(all->client(), GEARMAN_CLIENT_FREE_TASKS);
+
+  return TEST_SUCCESS;
+}
+
 static test_return_t pre_namespace(void *object)
 {
   client_test_st *all= (client_test_st *)object;
@@ -1070,6 +1172,8 @@ test_st unique_tests[] ={
 
 test_st regression_tests[] ={
   {"lp:768317", 0, regression_768317_test },
+  {"lp:785203 gearman_client_do()", 0, regression_785203_do_test },
+  {"lp:785203 gearman_client_do_background()", 0, regression_785203_do_background_test },
   {0, 0, 0}
 };
 
@@ -1135,20 +1239,26 @@ test_st gearman_task_tests[] ={
 collection_st collection[] ={
   {"gearman_client_st", 0, 0, tests},
   {"gearman_client_st chunky", pre_chunk, post_function_reset, tests}, // Test with a worker that will respond in part
-  {"gearman_strerror", 0, 0, gearman_strerror_tests},
-  {"gearman_task", 0, 0, gearman_task_tests},
-  {"gearman_task chunky", pre_chunk, post_function_reset, gearman_task_tests},
-  {"gearman_task namespace", pre_namespace, post_function_reset, gearman_task_tests},
+  {"gearman_strerror()", 0, 0, gearman_strerror_tests},
+  {"gearman_task_add_task()", 0, 0, gearman_task_tests},
+  {"gearman_task_add_task() chunky", pre_chunk, post_function_reset, gearman_task_tests},
+  {"gearman_task_add_task() namespace", pre_namespace, post_function_reset, gearman_task_tests},
+  {"gearman_task_add_task(GEARMAN_CLIENT_FREE_TASKS)", pre_namespace, post_function_reset, gearman_task_tests},
   {"unique", pre_unique, post_function_reset, unique_tests},
   {"gearman_client_do()", 0, 0, gearman_client_do_tests},
   {"gearman_client_do() namespace", pre_namespace, post_function_reset, gearman_client_do_tests},
-  {"gearman_execute chunky", pre_chunk, post_function_reset, gearman_execute_tests},
-  {"gearman_client_do_job_handle", 0, 0, gearman_client_do_job_handle_tests},
-  {"gearman_client_do_job_handle namespace", pre_namespace, post_function_reset, gearman_client_do_job_handle_tests},
-  {"gearman_client_do_background", 0, 0, gearman_client_do_background_tests},
+  {"gearman_client_do(GEARMAN_CLIENT_FREE_TASKS)", pre_free_tasks, post_free_tasks, gearman_client_do_tests},
+  {"gearman_client_do_job_handle()", 0, 0, gearman_client_do_job_handle_tests},
+  {"gearman_client_do_job_handle() namespace", pre_namespace, post_function_reset, gearman_client_do_job_handle_tests},
+  {"gearman_client_do_job_handle(GEARMAN_CLIENT_FREE_TASKS)", pre_free_tasks, post_free_tasks, gearman_client_do_job_handle_tests},
+  {"gearman_client_do_background()", 0, 0, gearman_client_do_background_tests},
+  {"gearman_client_do_background(GEARMAN_CLIENT_FREE_TASKS)", pre_free_tasks, post_free_tasks, gearman_client_do_background_tests},
   {"gearman_client_set_server_option", 0, 0, gearman_client_set_server_option_tests},
-  {"gearman_execute", 0, 0, gearman_execute_tests},
+  {"gearman_execute()", 0, 0, gearman_execute_tests},
+  {"gearman_execute(GEARMAN_CLIENT_FREE_TASKS)", pre_free_tasks, post_free_tasks, gearman_execute_tests},
+  {"gearman_execute() chunked return", pre_chunk, post_function_reset, gearman_execute_tests},
   {"gearman_execute_map_reduce()", 0, 0, gearman_execute_map_reduce_tests},
+  {"gearman_execute_map_reduce(GEARMAN_CLIENT_FREE_TASKS)", pre_free_tasks, post_free_tasks, gearman_execute_map_reduce_tests},
   {"gearman_command_t", 0, 0, gearman_command_t_tests},
   {"regression_tests", 0, 0, regression_tests},
   {"client-logging", pre_logging, post_logging, tests_log},