AsyncProgressWorker::OnProgress is invoked without arg sometimes #821

Activity

1. mhdawson commented on Nov 2, 2020

   

   mhdawson

   on Nov 2, 2020

   Member

   More actions

   @zbjornson can you share the code? Since you can't recreate with the sample, its probably something in the code that you've not shared in the issue.
2. zbjornson commented on Nov 4, 2020

   

   zbjornson

   on Nov 4, 2020

   Author

   More actions

   @mhdawson here's a repro minimally modified from the example (only Execute and OnProgress modified)

   #include <chrono>
   #include <thread>
   #include <string>
   
   using namespace Napi;
   
   struct ProgressInfo {
     float progress;
     const char* status;
   };
   
   class EchoWorker : public AsyncProgressWorker<ProgressInfo> {
       public:
           EchoWorker(Function& callback, std::string& echo)
           : AsyncProgressWorker(callback), echo(echo) {}
   
           ~EchoWorker() {}
   
       void Execute(const ExecutionProgress& progress) {
           for (uint32_t i = 0; i < 500; ++i) {
             for (uint32_t j = 0; j < 5; ++j) {
               ProgressInfo pi{i * j, "running"};
               printf("sending %f\n", 1.0f * i * j);
               progress.Send(&pi, 1);
             }
             std::this_thread::sleep_for(std::chrono::milliseconds(2));
           }
       }
   
       void OnOK() {
           HandleScope scope(Env());
           Callback().Call({Env().Null(), String::New(Env(), echo)});
       }
   
       void OnProgress(const ProgressInfo* data, size_t count) {
           if (count == 1) {
             printf("progress %f\n", data->progress);
           } else {
             printf("count is 0!\n"); abort();
           }
           HandleScope scope(Env());
           Callback().Call({Env().Null(), Env().Null(), Number::New(Env(), data->progress)});
       }
   
       private:
           std::string echo;
   };
   
   
   Value Echo(const CallbackInfo& info) {
       // We need to validate the arguments here
       Function cb = info[1].As<Function>();
       std::string in = info[0].As<String>();
       EchoWorker* wk = new EchoWorker(cb, in);
       wk->Queue();
       return info.Env().Undefined();
   }
   
   Napi::Object Init(Napi::Env env, Napi::Object exports) {
     exports.Set(Napi::String::New(env, "Echo"), Napi::Function::New(env, Echo));
     return exports;
   }

   mymodule.Echo("a", console.log);

   This prints something like:

   ...
   null null 892
   sending 0.000000
   sending 224.000000
   sending 448.000000
   sending 672.000000
   sending 896.000000
   progress 896.000000
   null null 896
   count is 0!
   Aborted (core dumped)
   
3. mhdawson commented on Nov 4, 2020

   

   mhdawson

   on Nov 4, 2020

   Member

   More actions

   Can you give me all the files for the cut down example needed to build/run?

   Possibly not relevant, I'm wondering if there is something going on with status part in

   struct ProgressInfo {
     float progress;
     const char* status;
   };

   progress.Send(&pi, 1) is going to make a copy of pi, but not a deep copy of the status field (that might be just fine but just not 100% of the lifetime for the string defined in the initializer). I see that is not referred elsewhere in the code, do you still see the problem if you remove that field from ProgressInfo?
4. zbjornson commented on Nov 4, 2020

   

   zbjornson

   on Nov 4, 2020

   Author

   More actions

   Sure, full repo here: https://gist.github.com/zbjornson/5edc22bb7381e4c2a1f00aee44abe4b7 (or git clone git@gist.github.com:5edc22bb7381e4c2a1f00aee44abe4b7.git).

   I pared it down further so it's just AsyncProgressWorker<float>.
5. legendecas commented on Nov 8, 2020

   

   legendecas

   on Nov 8, 2020

   Last edited by legendecas

   Member

   More actions

   @zbjornson thanks for your detailed re-produce and reporting here!

   I investigated the re-produce and found out the issue may be related to the timing of SendProgress been called during two async callback of thread-safe function:

   javascript thread	worker thread
   -	send progress (A), activate uv_async_t
   uv_async_t async callback before calling into module	-
   -	send progress (B), override (A) before being read, re-activate uv_async_t
   calling into module, AsyncProgressWorker read data (B) and clear out	-
   uv_async_t async callback before calling into module	-
   calling into module, AsyncProgressWorker reads nothing	-

   As far as I can tell, the AsyncProgressWorker guarantees at least once of the progress callback. However, in the issue, the callback was called with unexpected data (data with nullptr and count with 0 - never been sent with that progress), so this is a confirmed bug.

   I'll file a fix.

   👍React with 👍1Reacted by Zach Bjornson
6. legendecas

   added

   bug

   on Nov 8, 2020
7. zbjornson commented on Nov 11, 2020

   

   zbjornson

   on Nov 11, 2020

   Author

   More actions

   Thank you!
8. mhdawson

   closed this as completedin 86feeebon Dec 14, 2020
9. kevindavies8

   added a commit that references this issue on Aug 24, 2022

   src: empty data OnProgress in AsyncProgressWorker

   ...

   41d35d5
10. Marlyfleitas

    added a commit that references this issue on Aug 26, 2022

    src: empty data OnProgress in AsyncProgressWorker

    ...

    b770a61
11. wroy7860

    added a commit that references this issue on Sep 19, 2022

    src: empty data OnProgress in AsyncProgressWorker

    ...

    78d5c6a
12. johnfrench3

    added a commit that references this issue on Aug 11, 2023

    src: empty data OnProgress in AsyncProgressWorker

    ...

    1504beb