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

setParent breaks the Segment encapsulation #420

Open
xendo opened this issue Jan 20, 2025 · 1 comment
Open

setParent breaks the Segment encapsulation #420

xendo opened this issue Jan 20, 2025 · 1 comment

Comments

@xendo
Copy link

xendo commented Jan 20, 2025

setParent and setParentSegment methods are exposed as Susbegment's API, but they have undocumented side effects. To give you two examples:

  • when a new Subsegment is created it updates its parent's reference counter parentSegment.incrementReferenceCount(); when subsegment is ended it will decrement that counter: parentSegment.decrementReferenceCount() . If between a Subsegment start and end we call setParentSegment(), we will increment and decrement reference counters for different parent's which is always never what we really want.
  • when setParent is called it only updates the parent of the current subsegment but it doesn't update the subsegments of that parent. That means that the old parent will keep the subsegment and new parent will not get the subsegment

so the actual reparenting should look sth like this:

oldParent = subsegment.getParent();
subsegment.setParent(newParent);
subsegment.setParentId(newParent.getId());
oldParent.decrementReferenceCount();
oldParent.removeSubsegment(subsegment);
newParent.incrementReferenceCount();
newParent.addSubsegment(subsegment);

the problem with that code is that it's not atomic, which may lead to weird issues if, for example newParent reference counter goes to 0 at the same time we are doing reparenting.

We need to either update the setParent's documentation to accomodate for that, or make reparenting safe with regards to reference counting and subsegment lists.

This is related to the workaround for #419 that I'm working on.

@mxiamxia
Copy link

Yes. It is a problem if you want to reparent the subsegment in this way. Instead of manually reset the parent as workaround, can you try the subsegment.close() method which will remove the current subsegment from the context so the next subsegment will pick the upper level of segment as parent?

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

No branches or pull requests

2 participants