Opened on 12/02/2016 at 07:16:08 PM

Closed on 03/21/2017 at 02:38:46 PM

#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):



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.

Attachments (0)

Change History (5)

comment:1 Changed on 12/21/2016 at 08:25:00 PM by

  • Review URL(s) modified (diff)

Review 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 on 03/17/2017 at 05:06:52 PM 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 on 03/20/2017 at 02:08:50 PM by sergz

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

comment:4 Changed on 03/21/2017 at 02:17:18 PM by abpbot

comment:5 Changed on 03/21/2017 at 02:38:46 PM by sergz

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

@Eric, I think now 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.

Add Comment

Modify Ticket

Change Properties
as closed .
The resolution will be deleted. Next status will be 'reopened'.
to The owner will be changed from sergz.
Note: See TracTickets for help on using tickets.