Ancestors

Toot

Written by David Bureš on 2025-01-15 at 12:16

Guys, I'm having trouble with unexpected behavior in the package synchronization function for Cork

https://github.com/buresdv/Cork/blob/b82252552252dc595edca69c6ce55a8f76e0d50b/Cork/Logic/Installation%20%26%20Removal/Synchnorize%20Installed%20Packages.swift

Anyone free to help me with it? I'd really appreciate that

[#]swift #CorkApp

=> More informations about this toot | More toots from davidbures@mstdn.social

Descendants

Written by Matt Massicotte on 2025-01-15 at 12:19

@davidbures Tell me more, what’s going wrong?

=> More informations about this toot | More toots from mattiem@mastodon.social

Written by David Bureš on 2025-01-15 at 12:26

@mattiem First of all, thank you a lot for being available :)

This function is supposed to reload installed packages

It calls the same methods that are used for loading packages when Cork first starts. That works fine

It works fine almost every time, apart from one use case: When I try to call it after a package installation is finished. It then returns an empty array

This is where it's called during installation: https://github.com/buresdv/Cork/blob/b82252552252dc595edca69c6ce55a8f76e0d50b/Cork/Models/Package%20Installation/Installation%20Progress%20Tracker.swift#L53

I can't think of a reason why it does that :/

=> More informations about this toot | More toots from davidbures@mstdn.social

Written by David Bureš on 2025-01-15 at 12:29

@mattiem One more thing, this is where the package loading function is used to first load the packages: https://github.com/buresdv/Cork/blob/b82252552252dc595edca69c6ce55a8f76e0d50b/Cork/ContentView.swift#L292

=> More informations about this toot | More toots from davidbures@mstdn.social

Written by Matt Massicotte on 2025-01-15 at 12:32

@davidbures Ok, so you are saying one (or possibly both) of the internal calls to self.loadInstalledPackages is returning something unexpected is that right?

=> More informations about this toot | More toots from mattiem@mastodon.social

Written by Matt Massicotte on 2025-01-15 at 12:34

@davidbures I also want to take a moment to comment on some unrelated stuff, since I'm seeing it.

There are a few things in here that I think you should revisit. In particular, I see MainActor methods and explicit priorities for .task. Both of these are probably not want you want. I talk, briefly, about both of these things here: https://www.massicotte.org/problematic-patterns

=> More informations about this toot | More toots from mattiem@mastodon.social

Written by David Bureš on 2025-01-15 at 12:36

@mattiem Thank you, I will definitely read up on that when this problem is fixed :)

=> More informations about this toot | More toots from davidbures@mstdn.social

Written by David Bureš on 2025-01-15 at 12:35

@mattiem I think so.

In all cases, I internally call brewData.loadInstalledPackages. When that's called directly when I start up Cork, it returns the expected values (a parsed set of installed packages)

I'm using brewData.loadInstalledPackages in the synchronization function as well, to try to get an updated list of packages after a change has been made

When I use that same synchronization function after finishing up an installation, it unexpectedly returns an empty set

=> More informations about this toot | More toots from davidbures@mstdn.social

Written by Matt Massicotte on 2025-01-15 at 12:38

@davidbures ok so what makes you think the problem is in this function, and not in loadInstalledPackages?

=> More informations about this toot | More toots from mattiem@mastodon.social

Written by David Bureš on 2025-01-15 at 12:42

@mattiem As far as I have noticed, synchronizeInstalledPackages works properly for the operations I tested it with (for example, when I uninstall a package and use synchronizeInstalledPackages, it correctly returns the parsed packages)

It does this unexpected empty return only when I use synchronizeInstalledPackages as a part of the installPackage function

TBF, it might fail during other operations, but I haven't tried them because I don't know how to manually trigger them

=> More informations about this toot | More toots from davidbures@mstdn.social

Written by David Bureš on 2025-01-15 at 12:45

@mattiem Oh never mind, I just tested it in another workflow (https://github.com/buresdv/Cork/blob/b82252552252dc595edca69c6ce55a8f76e0d50b/Cork/Views/Installation/Sub-Views/Status%20Views/Binary%20Already%20Exists.swift#L53) and that also returns the unexpected empty set!

=> More informations about this toot | More toots from davidbures@mstdn.social

Written by Matt Massicotte on 2025-01-15 at 12:45

@davidbures Ok yes, but it isn't really doing a lot of work, it is just relaying results. I think you should turn your attention to that inner function because I bet that's where the problem is.

=> More informations about this toot | More toots from mattiem@mastodon.social

Written by David Bureš on 2025-01-15 at 12:48

@mattiem You're right, so this is the function we need to be looking at: https://github.com/buresdv/Cork/blob/b82252552252dc595edca69c6ce55a8f76e0d50b/Cork/Logic/Package%20Loading/Load%20Up%20Installed%20Packages.swift

I did my best to document it thoroughly because it's quite complex, does anything jump out at you?

=> More informations about this toot | More toots from davidbures@mstdn.social

Written by Matt Massicotte on 2025-01-15 at 12:55

@davidbures You’re right there’s a lot going on. But I’m not familiar with the domain so hard to say. You have a fair bit of logging in there and that’s handy.

But I’m pretty sure this is the area that needs your attention.

=> More informations about this toot | More toots from mattiem@mastodon.social

Written by David Bureš on 2025-01-15 at 12:57

@mattiem I will try to add even more logging to see where it goes wrong. Because it seems like there's a stage between it correctly loading the contents of the package folder, and it actually returning the results, that breaks.

=> More informations about this toot | More toots from davidbures@mstdn.social

Written by David Bureš on 2025-01-15 at 16:34

@mattiem I think I made some discoveries

I added more logging: https://github.com/buresdv/Cork/commit/b82252552252dc595edca69c6ce55a8f76e0d50b

And this is the log output: https://pastebin.com/Bg4zKRvL

I think the task group here might be messing it up: https://github.com/buresdv/Cork/blob/129f8bf99cceb2f685e2ffa125d029d75227caa1/Cork/Logic/Package%20Loading/Load%20Up%20Installed%20Packages.swift#L112

I can't even think straight about this :blobsweat: Do you see anything weird?

=> More informations about this toot | More toots from davidbures@mstdn.social

Written by Matt Massicotte on 2025-01-15 at 19:03

@davidbures It certainly seems like you are on the right track. I don’t immediately notice anything, but there’s a lot going on here. If you keep at this I think you’ll get it.

=> More informations about this toot | More toots from mattiem@mastodon.social

Written by David Bureš on 2025-01-16 at 08:29

@mattiem I think I figured it out.

I changed addTaskUnlessCancelled to addTask, and now it loads it properly. I had no idea why the task group was getting cancelled

=> View attached media

=> More informations about this toot | More toots from davidbures@mstdn.social

Written by Matt Massicotte on 2025-01-16 at 10:00

@davidbures Great!

It is run from a .task modifier? Maybe the owning view was being dismissed?

=> More informations about this toot | More toots from mattiem@mastodon.social

Written by Matt Massicotte on 2025-01-16 at 12:00

@davidbures Also, this is in that post I mentioned before, but I really think you should remove these explicit properties. They are extremely hard to reason about, and can have surprising effects on ordering and performance.

=> More informations about this toot | More toots from mattiem@mastodon.social

Written by David Bureš on 2025-01-16 at 12:54

@mattiem You’re right, that’s something I wanted to do when I have the room. Thanks a lot 🫡

=> More informations about this toot | More toots from davidbures@mstdn.social

Written by David Bureš on 2025-01-17 at 11:25

@mattiem I have a related question: in your article, you said that defining manual priorities for tasks tends to mess up system scheduling. What if I have a ton of tasks that get fired at the same time when the app starts, and I want to force the task for loading packages to have the highest priority?

Would it be a good idea to designate that task as high priority, and leave the rest without an explicit priority? The UI is in a loading state before that task finishes

=> More informations about this toot | More toots from davidbures@mstdn.social

Written by Matt Massicotte on 2025-01-17 at 11:33

@davidbures If you can fully explain why there is a benefit to doing it that way, and that it will not result in any priority inversions, then it could be worth it. But that’s hard to do.

=> More informations about this toot | More toots from mattiem@mastodon.social

Written by David Bureš on 2025-01-17 at 13:37

@mattiem I was wondering about the benefit… Cork is pretty much unusable until packages get loaded, so I wanted to tell the system “the task that loads packages needs to be completed first if possible” 🤔

=> More informations about this toot | More toots from davidbures@mstdn.social

Written by Matt Massicotte on 2025-01-17 at 14:20

@davidbures I cannot answer this question. But I have a feeling this is not helping. I’d just measure to see what specifically the system is doing, if you really want to dive in here.

=> More informations about this toot | More toots from mattiem@mastodon.social

Written by David Bureš on 2025-01-16 at 12:54

@mattiem I’ll look into it, it might be something like that

=> More informations about this toot | More toots from davidbures@mstdn.social