Skip to content

Replace SafeLoader monkey-patch with dedicated YAML loader subclass - #56

Merged
arne48 merged 2 commits into
ros-visualization:humblefrom
rafal-gorecki:fix/yaml-loader-isolation
Jun 10, 2026
Merged

Replace SafeLoader monkey-patch with dedicated YAML loader subclass#56
arne48 merged 2 commits into
ros-visualization:humblefrom
rafal-gorecki:fix/yaml-loader-isolation

Conversation

@rafal-gorecki

Copy link
Copy Markdown
Contributor

Bug

rqt_tf_tree crashes with RecursionError when the TF tree launched before nodes publishing tf.

File ".../rqt_tf_tree/dotcode_tf.py", line 100, in default_tftree_construct_mapping
    data = self.construct_mapping_org(node, deep)
[Previous line repeated 984 more times]
RecursionError: maximum recursion depth exceeded

Reproducer

  1. ros2 run rqt_tf_tree rqt_tf_tree
  2. Click Refresh.
  3. Launch some tf publisher
  4. Click Refresh.

Error fires automatically on first display when rqt is started before any TF
publisher exists; rqt internally retries dotcode generation, which triggers
the second invocation.

Why the fix works

Previous code mutated yaml.SafeLoader.construct_mapping globally on every
call and unconditionally saved the "original" into construct_mapping_org.
After the second call, the saved "original" was itself the previous override,
so the override called itself — recursion.

The fix replaces the runtime mutation with a dedicated
_TfTreeYamlLoader(yaml.SafeLoader) subclass that registers the
int-key-stringifying mapping constructor once, at module import. No per-call
mutation → no idempotency concern → bug fixed by construction. Bonus: the
global yaml.SafeLoader is no longer polluted for other modules in the same
process.

Comment thread src/rqt_tf_tree/dotcode_tf.py
@arne48

arne48 commented Jun 8, 2026

Copy link
Copy Markdown
Contributor

Thank you very much for your fix @rafal-gorecki.
All contributions/improvements are always highly appreciated.
It seems that the PR #58 also addressed this issue and was recently merged.
However, your approach might help to improve the readability/structure of the code.
So if you would rebase your PR we could also merge your solution.

@rafal-gorecki
rafal-gorecki force-pushed the fix/yaml-loader-isolation branch from 01ff01f to 414527e Compare June 9, 2026 07:38
@rafal-gorecki

Copy link
Copy Markdown
Contributor Author

Thanks @arne48!
Rebased onto latest humble. Since #58 already fixes the crash, this now sits on top of it as a structural improvement — replacing the global SafeLoader monkey-patch with a dedicated _TfTreeYamlLoader subclass, so the shared loader is no longer mutated.

@arne48

arne48 commented Jun 10, 2026

Copy link
Copy Markdown
Contributor

Thank you very much for the fast update.
This is a now a nice structural upgrade.

@arne48
arne48 merged commit ad6e041 into ros-visualization:humble Jun 10, 2026
2 checks passed
@rafal-gorecki
rafal-gorecki deleted the fix/yaml-loader-isolation branch June 11, 2026 07:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants