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

Fix breaking dependency #149

Merged
merged 4 commits into from
Jul 19, 2024
Merged

Fix breaking dependency #149

merged 4 commits into from
Jul 19, 2024

Conversation

NiklasNeugebauer
Copy link
Contributor

@NiklasNeugebauer NiklasNeugebauer commented Jul 16, 2024

Issue: With updated packages, running test_examples.py sporadically creates empty rosys.config.json (persistence for the Config class), which breaks the examples that are run afterwards.

My best guess is that there is some kind of race condition where the backup function was called and started clearing the file but did not finish before the task was cancelled.
It seems to me that the errors never impacted normal programs.

I was able to (accidentally) lower the chance of the empty write happening by adding some debug prints (see ef750a9).
In the end, I decided to first write to a .tmp file and then rename the file. If the write is somehow cancelled, then only the tmp file will be empty.

Opinions?
@rodja @falkoschindler

@NiklasNeugebauer NiklasNeugebauer marked this pull request as draft July 16, 2024 11:39
@falkoschindler falkoschindler changed the title [DRAFT] fix breaking dependency Fix breaking dependency Jul 18, 2024
@falkoschindler falkoschindler marked this pull request as ready for review July 18, 2024 11:59
@falkoschindler falkoschindler self-requested a review July 18, 2024 11:59
Copy link
Contributor

@falkoschindler falkoschindler left a comment

Choose a reason for hiding this comment

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

Ok, the solution seems reasonable.
It just bugs me that we still don't know which package upgrade caused the problem. And we can't be 100% sure it won't affect backup data on real devices.

What do you think, @rodja?

Copy link
Member

@rodja rodja left a comment

Choose a reason for hiding this comment

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

Yes, thats fine with me. I don't have a better idea...

@rodja rodja merged commit f33d2fe into main Jul 19, 2024
3 checks passed
@rodja rodja deleted the find-breaking-dependency-update branch July 19, 2024 03:32
@falkoschindler falkoschindler added the bug Something isn't working label Jul 19, 2024
@falkoschindler falkoschindler added this to the 0.12.0 milestone Jul 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants