grafana / grafana/pyroscope-python

Fork safety

オープン
#122 コメント 0 件 リアクション 0 件 担当者 0 名 GitHub で見る
主要言語
Rust
スター
4
フォーク
2
平均マージ
2日 4時間
マージ済み PR(30日)
5

説明

During review of https://github.com/grafana/pyroscope-python/pull/83 we found our profiler is not safe for use if the target application is using forks after the pyroscope python SDK has started few threads.

A good description of the class of the problem can be found here https://www.qnx.com/developers/docs/8.0/com.qnx.doc.neutrino.getting_started/topic/s1_procs_Multithreaded_fork.html

> POSIX says that following a [fork()](https://www.qnx.com/developers/docs/8.0/com.qnx.doc.neutrino.lib_ref/topic/f/fork.html), the child's memory state is a copy of the parent's, but the child contains only a single thread; any other threads in the parent aren't duplicated in the child. If one of those parent threads was in the middle of manipulating a data structure when the fork() happened, that data structure will be in an undefined, possibly inconsistent, state in the child following the fork(). Regular mutexes can be used to protect different threads from interfering with each other, but protecting against fork() requires the forking code to be aware of the internals of your data structure synchronization.
>
> For example, suppose the parent process has two threads, T1 and T2, and T2 has locked a mutex. If T1 calls fork(), the child process is a copy of the parent, including any mutexes, but with only one thread. If the thread in the child tries to lock the mutex, the operation fails because T2 has it locked—and there is no T2 in the child process, so no one can unlock the mutex. This can lead to a deadlock.
>
> The simplest solution is to call fork() only from a single-threaded process, or to call it before you create additional threads. But, if you insist on using fork() in a multithreaded process, there are some things you can do:
>
> - Use at-fork handlers
> - Use forksafe mutexes
> - Call exec*() soon after forking. POSIX requires that the child process use only functions that are async-signal-safe until it calls one of the exec*() functions.

Another good read https://discuss.python.org/t/switching-default-multiprocessing-context-to-spawn-on-posix-as-well/21868

We've attempted to https://github.com/grafana/pyroscope-python/pull/119 . Few problems were found and workarounded, such as logging stderr lock, glibc dns resolver (only fixed on linux, not mac), http client internal locking(probably from tokio). The running agent memory was leaked in the child. There is at least one mutex that was not addressed in this PR inside the openssl errors table (see the PR description).

The cpython does attempt to recover the interpreter state in the child, but issues a warning about deadlocks. https://github.com/python/cpython/issues/100228

It has been decided to postpone the attempts to recover forked children and abandon #119 until we see need. Instead we're going to issue a warning and document this as unsupported and invalid use of the pyroscope python sdk with an examples how to avoid these problems (start the pyroscope after the forks, or stop the pyroscope before forking and ensure no threads are interacting with pyroscope during the fork

コントリビューションガイド

このリポジトリのコントリビューションガイドは索引されていません

調査の方向性

まず、pull request #119と、このissueで説明されているSDKのfork関連の動作を確認します。profilerがスレッドを開始および停止する箇所と、ユーザー向けの警告およびドキュメントが管理されている箇所を特定します。forkされた子プロセスでの使用がサポートされていないことをユーザーに警告し、安全な代替手段として、profilerを開始する前にforkするか、forkする前にprofilerを停止することを文書化できれば完了です。

索引モデルが issue の本文から書いたものです。

評価

技術スタック
python, rust
領域
documentation, observability-sre
issue の種類
ドキュメント
難易度
3/5
見積もり時間
1〜2日
活発さ
静か
明瞭さ
おおむね明確
初心者へのやさしさ
55/100

新しい issue をメールで受け取る

初心者向けの GitHub issue を短くまとめたダイジェスト。