Opened 4 years ago

Last modified 23 months ago

#3595 closed defect

Get rid of detached threads — at Version 9

Reported by: sergz Assignee:
Priority: P2 Milestone:
Module: Libadblockplus Keywords:
Cc: fhd, rjeschke, oleksandr, eric@…, asmirnov, hfiguiere Blocked By: #4688, #4692
Blocking: #3593 Platform: Unknown / Cross platform
Ready: no Confidential: no
Tester: Unknown Verified working: no
Review URL(s):

https://codereview.adblockplus.org/29367507/
https://codereview.adblockplus.org/29367522/
https://codereview.adblockplus.org/29395640/

Description (last modified by sergz)

adblockplus uses timers (setTimeout) (with very long firing intervals). Current implementation in libadblockplus uses free running timer threads, each thread is represented as an instance of a TimeoutThread class which holds JsValuePtr function; and JsValueList functionArguments; which hold JsEnginePtr jsEngine; that holds isolate.

It results in some race conditions in tests as well as in some memory leakages (see #3593).

What to change

Stop keeping strong references to JsEngine and stop all timers when JsEngine is being destroyed.

Change History (9)

comment:1 Changed 4 years ago by sergz

  • Blocking 3593 added

comment:2 Changed 4 years ago by sergz

  • Description modified (diff)

What do you think about using of libuv for async IO and timers? It should allow us to get rid of detached threads and avoid having of any internal resources longer than the scope of JsEngine among currently supported platforms.

comment:3 Changed 3 years ago by asmirnov

  • Cc Anton added

comment:4 Changed 3 years ago by sergz

  • Blocked By 4688 added

comment:5 Changed 3 years ago by sergz

  • Blocked By 4692 added

comment:6 Changed 3 years ago by eric@…

  • Review URL(s) modified (diff)
  • Status changed from new to reviewing

comment:7 Changed 3 years ago by eric@…

The solution in ​https://codereview.adblockplus.org/29367507/, and continued in ​https://codereview.adblockplus.org/29367522/, is to not detach threads. That means setting up an infrastructure to be able to call join() on them somewhere. What it does not mean for those changes is eliminating the threads entirely. That might be desirable in its own right, but it would be a separate change.

Last edited 3 years ago by eric@… (previous) (diff)

comment:8 Changed 3 years ago by sergz

  • Cc asmirnov added; Anton removed

comment:9 Changed 3 years ago by sergz

  • Review URL(s) modified (diff)
Note: See TracTickets for help on using tickets.