search

microsoft / cpp_client_telemetry

1DS C++ SDK
Apache License 2.0
87 stars 49 forks source link

Task pools, multithreading and reliable task cancellation logic #286

Open maxgolov opened 4 years ago

maxgolov commented 4 years ago

Need to discuss the following design ideas:

Single-threaded vs. multi-threaded worker pool.

Currently SDK is single-threaded worker pool + WinInet thread pool for HTTP requests on Windows. In some service scenarios it is more advantageous to maintain more than one background worker thread.

Single vs. multiple pools for multiple SDK (LogManager) instances

Currently SDK shares one thread pool between many several LogManager instances. In some scenarios it might be best to have independent thread pools for each "channel" (for each LogManager instance).

Ability to track a task by unique TaskId

Rough-in proposed implementation is added in PR #266, but currently it's not used for task cancellation and task look-up. raw task ptr used instead. This is not robust and potentially error-prone in some corner cases, esp. if a new task gets allocated on heap with the same ptr while we idle-waited with sleep on task completion.

Ability to associate a task with its owner LogManager

Currently tasks are not in any way, shape or form are linked back to their parent LogManagers and their deferred task handles. It would be of benefit to consolidate (join, terminate, run to completion or cancel) all outstanding tasks that belong to certain instance of LogManager on that instance shutdown, so that we have no orphan tasks running.

larvacea commented 4 years ago

Thread Pools The Android implementation uses a Java-world fixed thread pool (infinite queue feeding two threads) to run https requests.

Unique task id It would be delightful if the unique id were uint64_t rather than std::string (request IDs are std::string, and it would have been delightful if those were uint64_t). I would also vote against naked pointers, and in favor of std::shared_ptr for lifetime management and fewer use-after-free bugs.

Heap allocation Objects of uncertain lifetime should never be allocated on the heap. Use after free, not good.

Cancellation Asynchronous-cancellation is a fruitful source of bugs and the endless fun of diagnosing concurrency, race conditions, and use-after-free memory corruption. So just as with the joys of task lifetime, as soon as tasks can touch log manager instances, extant tasks have to be able to keep their log manager alive until they die (either from cancellation, exception, or completion).