Opened 2 years ago

Closed 2 years ago

#4694 closed change (fixed)

JsEngine::{SetEventCallback,RemoveEventCallback,TriggerEvent} should be thread safe.

Reported by: sergz Assignee: sergz
Priority: P2 Milestone:
Module: Libadblockplus Keywords:
Cc: fhd Blocked By:
Blocking: #4948 Platform: Unknown / Cross platform
Ready: no Confidential: no
Tester: Unknown Verified working: no
Review URL(s):

https://codereview.adblockplus.org/29389576/

Description

Background

Beside the fact that user can call these methods from different threads, it can happen that while a user calls (or calls indirectly by calling corresponding methods of FilterEngine), e.g. SetEventCallback some background operation calls TriggerEvent and they both access JsEngine::eventCallbacks while the former modifies it.

What to change

Use JsContext instance as synchronization sentry in these methods.

Change History (5)

comment:1 Changed 2 years ago by eric@…

  • Review URL(s) modified (diff)

Review https://codereview.adblockplus.org/29369479/ uses std::mutex for synchronization.

Using JsContext would gratuitously convoluted at best. The only synchronization mechanism in JsContext is its member v8::Locker, which only synchronizes v8 engine operations. It would be possible to use v8::Locker by evaluating some JS code (to get into the engine and avail ourselves of its lock) and then calling back out it back to our code. It's much, much easier just to use standard C++ synchronization (with C++11).

comment:2 Changed 2 years ago by sergz

I agree that JsContext is too much and we should rather use only v8::Locker.

The approach with using of std::mutex has its own advantages, however creating the issue I thought that v8::Locker might be more efficient here during the main work of application because we usually configure callbacks only once, mostly before running of JS code and experience calls of TriggerEvent many times later. With v8::Locker TriggerEvent might be a bit faster because v8::Locker firstly checks whether it's already locked by current thread what is indeed happening all the time for TriggerEvent because it's called only from JavaScript which has already acquired that lock in the current thread. So, acquiring of std::mutex here can be a bit heavier operation. Though, I'm not sure whether we can actually observer the impact of different approaches.

comment:3 Changed 2 years ago by sergz

  • Blocking 4948 added
  • Owner set to sergz
  • Priority changed from P3 to P2
  • Review URL(s) modified (diff)

comment:5 Changed 2 years ago by sergz

  • Resolution set to fixed
  • Review URL(s) modified (diff)
  • Status changed from new to closed

@Eric, I think now https://codereview.adblockplus.org/29369479/ is rather a refactoring and should be a part of another issue or as noissue, so I'm removing it from the field "Review URL(s)". JIC, I don't have objections about extracting the functionality into a separate class.

Note: See TracTickets for help on using tickets.