Skip to content

Conversation

@ayshih
Copy link
Member

@ayshih ayshih commented May 21, 2025

#7437 added a warning when both the assume_spherical_screen() (now, SphericalScreen()) and propagate_with_solar_surface() context managers were used together, but there is a bug in the implementation of tracking active contexts that gets confused if a given context manager is used multiple times in a nested structure, e.g.:

with SphericalScreen(...), propagate_with_solar_surface():
    with propagate_with_solar_surface():
        pass

    # Context tracker thinks that the first propagate_with_solar_surface() is no longer active
    # because the second propagate_with_solar_surface() has exited
    ...

This PR overhauls the internal tracking of context managers to use the data structure of a stack instead of an unordered set (which was peculiarly implemented as a dictionary of True/False values). Now the tracking respects the nesting of contexts and can handle repeated contexts. Plus, the contexts are stored as fully qualified names, which should avoid potential collisions.

@ayshih ayshih added util Issues relating to sunpy.util backport 7.0 on-merge: backport to 7.0 labels May 21, 2025
@ayshih ayshih force-pushed the active_contexts_stack branch from e5b57e1 to 48a9d37 Compare May 21, 2025 18:43
@ayshih
Copy link
Member Author

ayshih commented May 21, 2025

While conceptually this can/should be backported to 6.0/6.1, it does change quite a bit with private behavior, so I'm fine with keeping it to 7.0. The likelihood of this bug affecting users in the wild is low, and the consequence is simply that a warning is not being emitted when it should be.

@nabobalis nabobalis added this to the 7.0.0 milestone May 21, 2025
@ayshih ayshih marked this pull request as ready for review May 21, 2025 19:06
Comment on lines +38 to +39
if (removed := ACTIVE_CONTEXTS.pop()) != self._context_name:
raise RuntimeError(f"Cannot remove {self._context_name} from tracking stack because {removed} is last active.")
Copy link
Member

Choose a reason for hiding this comment

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

Codecov says no test? I am not sure this is worth testing but I leave that up to you.

Copy link
Member Author

@ayshih ayshih May 28, 2025

Choose a reason for hiding this comment

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

A test feels a little unpalatable to write because this condition should never be satisfied with normal code. Someone would need to intentionally modify the tracking registry directly while within contexts, and if someone is going to do that, basically all bets are off.

@ayshih ayshih merged commit 4bea55c into sunpy:main May 28, 2025
28 of 29 checks passed
@ayshih ayshih deleted the active_contexts_stack branch May 28, 2025 17:21
meeseeksmachine pushed a commit to meeseeksmachine/sunpy that referenced this pull request May 28, 2025
nabobalis added a commit that referenced this pull request May 28, 2025
…1-on-7.0

Backport PR #8211 on branch 7.0 (Fixed a bug with tracking our active contexts)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backport 7.0 on-merge: backport to 7.0 util Issues relating to sunpy.util

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants