Skip to content

Change IPeak python export to be shared_ptr instead of raw pointer#41876

Open
rosswhitfield wants to merge 2 commits into
mantidproject:mainfrom
rosswhitfield:python_ipeak
Open

Change IPeak python export to be shared_ptr instead of raw pointer#41876
rosswhitfield wants to merge 2 commits into
mantidproject:mainfrom
rosswhitfield:python_ipeak

Conversation

@rosswhitfield

@rosswhitfield rosswhitfield commented Jul 22, 2026

Copy link
Copy Markdown
Member

Description of work

Fix segfault when accessing Peak from python after PeaksWorkspace has been deleted. This bug has existed in Mantid since at least Mantid 3.10.

By changing IPeak to be a shared_ptr the peaks workspace will exist as long a the python Peak's continue to exist. So this will fixes the segfault.

Closes #31345

To test:

This causes a segfault that should now be fixed

import time
from mantid.simpleapi import CreatePeaksWorkspace, DeleteWorkspace
peaks=CreatePeaksWorkspace(OutputType="LeanElasticPeak")
p=peaks.getPeak(0)
print(p.getScattering())
DeleteWorkspace(peaks)
time.sleep(1)
print(p.getScattering())

Reviewer

Your comments will be used as part of the gatekeeper process. Comment clearly on what you have checked and tested during your review. Provide an audit trail for any changes requested.

As per the review guidelines:

  • Is the code of an acceptable quality? (Code standards/GUI standards)
  • Has a thorough functional test been performed? Do the changes handle unexpected input/situations?
  • Are appropriately scoped unit and/or system tests provided?
  • Do the release notes conform to the guidelines and describe the changes appropriately?
  • Has the relevant (user and developer) documentation been added/updated?
  • If the PR author isn’t in the mantid-developers or mantid-contributors teams, add a review comment rerun ci to authorize/rerun the CI

Gatekeeper

As per the gatekeeping guidelines:

  • Has a thorough first line review been conducted, including functional testing?
  • At a high-level, is the code quality sufficient?
  • Are the base, milestone and labels correct?

@github-actions github-actions Bot added this to the Release 7.0 milestone Jul 22, 2026
@rosswhitfield rosswhitfield added Bug Issues and pull requests that are regressions or would be considered a bug by users (e.g. crashing) Single Crystal Issues and pull requests related to single crystal labels Jul 22, 2026
@rosswhitfield
rosswhitfield requested a review from Copilot July 22, 2026 01:59

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

This PR updates the Mantid Python bindings for IPeaksWorkspace.getPeak() (and peak iteration) so that Python IPeak objects hold shared ownership of the originating workspace, preventing use-after-free crashes when the workspace is deleted from the ADS. This addresses the long-standing Python segmentation fault described in #31345.

Changes:

  • Changed the IPeaksWorkspace.getPeak() Python export to return an aliasing std::shared_ptr<IPeak> that co-owns the workspace.
  • Updated the Python iterator for peaks workspaces to return the same shared-ownership peak handles.
  • Added Python unit tests to verify peak usability after DeleteWorkspace and that peak modifications remain reflected in the workspace.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File Description
Framework/PythonInterface/mantid/api/src/Exports/IPeaksWorkspace.cpp Updates Boost.Python bindings to return peaks as aliasing shared_ptr handles and adjusts iteration to preserve workspace lifetime.
Framework/PythonInterface/mantid/api/src/Exports/IPeak.cpp Registers std::shared_ptr<IPeak> for Boost.Python conversions to support the new return type.
Framework/PythonInterface/test/python/mantid/api/IPeaksWorkspaceTest.py Adds regression tests covering post-deletion peak access and ensuring workspace-reflective peak mutation semantics.

@rosswhitfield
rosswhitfield marked this pull request as ready for review July 22, 2026 04:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Bug Issues and pull requests that are regressions or would be considered a bug by users (e.g. crashing) Single Crystal Issues and pull requests related to single crystal

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Python IPeak Segmentation fault after PeaksWorkspace is deleted

2 participants