Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Bug]: multiple event loop thread after reload page cause 'napi_call_threadsafe_function' event missing #30122

Closed
3 tasks done
lishunbo opened this issue Jul 14, 2021 · 4 comments
Labels
13-x-y blocked/need-repro Needs a test case to reproduce the bug bug 🪲 has-repro-comment Issue has repro in comments

Comments

@lishunbo
Copy link

Preflight Checklist

Electron Version

13.1.6

What operating system are you using?

Windows

Operating System Version

Windows 10 家庭中文版 version 20H2

What arch are you using?

x64

Last Known Working Electron version

No response

Expected Behavior

After 'napi_call_threadsafe_function' in worker thread, reander thread will call 'NAPIThreadsafeFunctionCallJS' ASAP after reload window

Actual Behavior

After 'napi_call_threadsafe_function' in worker thread, reander thread not call 'NAPIThreadsafeFunctionCallJS' any more after reload window

Testcase Gist URL

No response

Additional Information

seems like multiple electron event loop cause missmatch writer and reader
before_reload_window
before_reload_threads
after_reload_window
after_reload_threads

addon code here:

 napi_threadsafe_function ts_func_;
std::thread worker_;
std::atomic<bool> exit_(true);

void NAPIThreadFinalizeCB(napi_env env, void* finalize_data,
                          void* finalize_hint) {
  Log::Warn(TAG_FUNC) << "begin" << std::endl;

  ts_func_ = nullptr;

  Log::Warn(TAG_FUNC) << "end" << std::endl;
}

void NAPIThreadsafeFunctionCallJS(napi_env env, napi_value js_callback,
                                  void* context, void* data) {
  napi_handle_scope scope;
  napi_status status = napi_open_handle_scope(env, &scope);
  if (status != napi_ok) return;

  static int js_cb_cnt = 0;
  Log::Info(TAG_FUNC) << js_cb_cnt++ << std::endl;

  napi_value obj;
  napi_get_undefined(env, &obj);

  napi_value ret;
  status = napi_call_function(env, obj, js_callback, 0, nullptr, &ret);
  if (status != napi_ok) {
    bool excep;
    napi_is_exception_pending(env, &excep);
    if (excep) {
      Log::Error(TAG_FUNC) << "call func pendding exception:" << std::endl;
    } else {
      const napi_extended_error_info* err;
      napi_get_last_error_info(env, &err);
      std::string em = "";
      if (err->error_message != nullptr) {
        em = err->error_message;
      }
      Log::Error(TAG_FUNC) << "call func failed:" << em << status << std::endl;
    }
  }
  status = napi_close_handle_scope(env, scope);
  if (status != napi_ok) {
    const napi_extended_error_info* err;
    napi_get_last_error_info(env, &err);
    Log::Error(TAG_FUNC) << "napi_close_handle_scope failed:"
                         << err->error_message << std::endl;
    return;
  }
}

void ThreadFunc() {
  napi_acquire_threadsafe_function(ts_func_);
  while (!exit_) {
    std::this_thread::sleep_for(std::chrono::milliseconds(100));
    static int js_cb_cnt = 0;
    Log::Info(TAG_FUNC) << js_cb_cnt++ << std::endl;
    napi_status status = napi_call_threadsafe_function(
        ts_func_, nullptr,
        napi_threadsafe_function_call_mode::napi_tsfn_blocking);
    if (status != napi_ok) {
      if (status == napi_closing) {
        break;
      }
      Log::Error(TAG_FUNC) << "napi_call_threadsafe_function failed:" << status
                           << std::endl;
      break;
    }
  }
  napi_release_threadsafe_function(ts_func_, napi_tsfn_abort);
  Log::Info(TAG_FUNC) << "exiting thread" << std::endl;
}

napi_value setCallBack(napi_env env, napi_callback_info args) {
  Log::Info(TAG_FUNC) << std::endl;

  size_t argc = 1;
  napi_value argv[1];
  napi_value thiz;
  napi_status status = napi_get_cb_info(env, args, &argc, argv, &thiz, nullptr);
  if (status != napi_ok) {
    napi_throw_error(env, nullptr,
                     "kAddonCommonError[AddonCommonErrorNo::kAddonError]");
    return nullptr;
  }

  if (!exit_) {
    return nullptr;
  }

  napi_valuetype cb_type = napi_undefined;
  napi_typeof(env, argv[0], &cb_type);
  if (cb_type != napi_function) {
    napi_throw_error(env, nullptr, "invalid argument, expect function");
    return nullptr;
  }

  napi_value resource, resourcename;

  status = napi_create_object(env, &resource);
  status = napi_create_string_utf8(env, "demo_resource_name", NAPI_AUTO_LENGTH,
                                   &resourcename);

  napi_create_threadsafe_function(env, argv[0], resource, resourcename, 50, 1,
                                  nullptr, NAPIThreadFinalizeCB, nullptr,
                                  NAPIThreadsafeFunctionCallJS, &ts_func_);

  exit_ = false;
  worker_ = std::thread(ThreadFunc);
  return nullptr;
}

napi_value release(napi_env env, napi_callback_info args) {
  exit_ = true;
  if (worker_.joinable()) worker_.join();

  Log::None(TAG_FUNC) << std::endl;

  return nullptr;
}

napi_value init(napi_env env, napi_value exports) {
  napi_status status;

  napi_value fn, fn2;
  status = napi_create_function(env, nullptr, 0, setCallBack, nullptr, &fn);

  status = napi_create_function(env, nullptr, 0, release, nullptr, &fn2);

  status = napi_set_named_property(env, exports, "setCallBack", fn);
  if (status != napi_ok) return nullptr;

  status = napi_set_named_property(env, exports, "release", fn2);
  if (status != napi_ok) return nullptr;

  return exports;
}

js code here:

const addon = require('../build/Release/demo_threadfunc_addon.node');

function DemoCallback() {
  console.log('js callback');
}

function startCallBack() {
  console.warn('click button');
  addon.setCallBack(DemoCallback);
}

window.onload = function () {
  document
    .getElementById('startAction')
    .addEventListener('click', startCallBack);
};

window.addEventListener('unload', function (event) {
  console.warn('window unload');
  addon.release();
});
@lishunbo
Copy link
Author

after dive in the source code, find a argument will effect this event_loop is create.
reuse_renderer_process
and the workaround is this:

app.allowRendererProcessReuse = false;

simple and valid, but it should support reuse process scene.

@jkleinsc jkleinsc added has-repro-comment Issue has repro in comments 13-x-y labels Jul 14, 2021
@codebytere
Copy link
Member

Unfortunately, we need to do that: #25869

I'll see what i can do, also perhaps cc @indutny and @deepak1556

@codebytere
Copy link
Member

Thanks for reporting this and helping to make Electron better!

Because of time constraints, triaging code with third-party dependencies is usually not feasible for a small team like Electron's.

Would it be possible for you to make a standalone testcase with only the code necessary to reproduce the issue? For example, Electron Fiddle is a great tool for making small test cases and makes it easy to publish your test case to a gist that Electron maintainers can use.

I'm adding the blocked/need-repro label for this reason. After you make a test case, please link to it in a followup comment. This issue will be closed in 10 days if the above is not addressed.

@codebytere codebytere added the blocked/need-repro Needs a test case to reproduce the bug label Jul 29, 2021
@lishunbo
Copy link
Author

lishunbo commented Aug 6, 2021

Sorry for being late, I'm on leave these days.
I'll do the test cases later, and close this issue for now

@lishunbo lishunbo closed this as completed Aug 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
13-x-y blocked/need-repro Needs a test case to reproduce the bug bug 🪲 has-repro-comment Issue has repro in comments
Projects
None yet
Development

No branches or pull requests

3 participants