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

Deal with race condition. #894

Merged
merged 2 commits into from Jun 21, 2021
Merged

Conversation

zeth
Copy link
Contributor

@zeth zeth commented Jun 21, 2021

We cannot call self._model.requestNewID() if self._model doesn't exist yet.

It seems to work like this. I don't know if the guard could just be around self.setData?

@TheJackiMonster
Copy link
Collaborator

I think it should be fine like this because I don't think the case of calling this method without a model being assigned may happen at all. Though I'm not sure if it does. That's one of my core problems with the current code base. We have way too many methods and Qt implicitly uses multi-threading at some instances which allows non-debuggable breaking code. ^^'

For example getUniqueID() gets called from setModel() and checkIDs(). I'm not certain but I think setModel() will only be called with a model excluding None. I'm not sure about checkIDs() though but the break fixed here may never happen. However the code allows it to happen.

So I would much more appreciate having less functions but them being safer. That's mostly the reason I refactor much more code than may be necessary in my fork, splitting up data uses and making access more clear. ^^

@zeth
Copy link
Contributor Author

zeth commented Jun 21, 2021

I did get a crash from this situation. I think it's because we are not totally duplicate id safe yet when cutting and pasting scenes. I think how I have done it here might be best as if you already have duplicate id, making a load of children with messed up ids makes it worse.

@TheJackiMonster
Copy link
Collaborator

I think how I have done it here might be best as if you already have duplicate id, making a load of children with messed up ids makes it worse.

Yes that makes sense. Currently in my refactored code the IDs should not be able to becomes duplicates to fix those issues. But this MR could improve the current situation a little.

@TheJackiMonster TheJackiMonster merged commit 4bfd663 into olivierkes:develop Jun 21, 2021
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.

None yet

2 participants