grafana / grafana/pyroscope-python
Fork safety
- Langage dominant
- Rust
- Étoiles
- 4
- Forks
- 2
- Merge moyen
- 2 j 4 h
- PR mergées (30 j)
- 5
Description
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
Guide de contribution
Aucun guide de contribution indexé pour ce dépôt
Piste de recherche
Commencez par examiner la pull request #119 et le comportement du SDK lié à fork décrit dans cette issue. Identifiez où le profiler démarre et arrête ses threads, et où sont gérées les avertissements destinés aux utilisateurs ainsi que la documentation. Le travail est terminé lorsque les utilisateurs sont avertis que l’utilisation dans un processus enfant créé par fork n’est pas prise en charge et que les alternatives sûres sont documentées : effectuer le fork avant de démarrer le profiler, ou l’arrêter avant de forker.
Rédigé par le modèle d'indexation à partir du texte de l'issue.
Évaluation
- Stack technique
- python, rust
- Domaine
- documentation, observability-sre
- Type d'issue
- Documentation
- Difficulté
- 3/5
- Temps estimé
- 1-2 jours
- Activité
- Calme
- Clarté
- Plutôt claire
- Accessibilité débutants
- 55/100