Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Avoid crash when converting dict with circular reference #74

Merged
merged 1 commit into from
Dec 9, 2024

Conversation

dbaston
Copy link
Contributor

@dbaston dbaston commented Dec 6, 2024

Fixes #73

Copy link
Collaborator

@martinRenou martinRenou left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Neat thanks!

Weird the CI is not picking it up

@martinRenou
Copy link
Collaborator

Damn with their new UI the button to approve workflow for new contributors is buried deep behind tabs and buttons

Copy link
Collaborator

@martinRenou martinRenou left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot! I just have a small concern, otherwise I think it looks good

include/pybind11_json/pybind11_json.hpp Outdated Show resolved Hide resolved
Copy link
Collaborator

@martinRenou martinRenou left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Neat! Thanks 🚀

@martinRenou martinRenou merged commit f00247d into pybind:master Dec 9, 2024
12 checks passed
@slayoo
Copy link

slayoo commented Dec 10, 2024

@dbaston, thank you for this fix!
We've bumped pybind11_json version to 0.2.15 and enabled a unit test that features a circular dict. Intriguingly, the behavior, from Python perspective, differs depending on the platform. On Apple Silicon (ARM64) macos-14, we get:

libc++abi: terminating due to uncaught exception of type std::runtime_error: Circular reference detected
Fatal Python error: Aborted

Current thread 0x00000001f6bc0f40 (most recent call first):
  File "/Users/runner/work/PyPartMC/PyPartMC/tests/test_aero_mode.py", line 304 in test_fixed_segfault_case_on_circular_reference
test_aero_mode.py::TestAeroMode::test_fixed_segfault_case_on_circular_reference 

on all other platforms, we get Python's TypeError indicating inconsistent function argument if the argument is a JSON containing a circular reference:

>       ppmc.AeroMode(aero_data, fishy_ctor_arg)
E       TypeError: __init__(): incompatible constructor arguments. The following argument types are supported:
E           1. _PyPartMC.AeroMode(arg0: _PyPartMC.AeroData, arg1: json)
E       
E       Invoked with: <_PyPartMC.AeroData object at 0x7ceea3fe04b0>, {'test_mode': {'mass_frac': [{'H2O': [1]}, [...]], 'diam_type': 'geometric', 'mode_type': 'log_normal', 'num_conc': 100.0, 'geom_mean_diam': 2e-06, 'log10_geom_std_dev': 0.2041199826559248}}

Any hints on why this behavior differs? Thanks

@dbaston
Copy link
Contributor Author

dbaston commented Dec 10, 2024

Any hints on why this behavior differs?

I'm not sure. It looks like the pybind11_json tests are running successfully on ARM64. You could try running the pybind11_json tests in your own CI to see if the problem appears there. You could also try provoking an exception that does not involve this PR, by trying to convert an unsupported type to JSON, and seeing if it is successfully caught.

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.

segfault when casting dictionary with circular reference to nlohmann::json
3 participants