cycle creation issue with new weakdeps code

[vtjnash]

Originally posted by me in #48513 (comment). Now making this an issue, since I think this really will need to be release blocking, as seems like possibly a significant reliability issue and heisenbug that is creeping upon the ecosystem.

Originally, I had that as the implementation and you went via Parent.Other to get dependencies. That however excluded the case where you wanted to load a package in an extension that was not a trigger nor was it loaded into Parent. This seemed to be a quite common desire so I allowed that. -Kristoffer

We observed recently that this should never have been allowed. It would be very useful, if it was valid. But it creates a cycle in the loading graph, which leads to unpredictable deadlocks, and causing the extension to sometimes hang (v1.9) or error (master today). It turns out that we must forbid this (for now), to prevent such unreliable behavior from catching users unawares and making PkgEval unreliable. We can re-evaluate later if we want to design a solution for it to reallow it later. Of note, it also must be forbidden (aka strongly discouraged) from loading those extra, unexpected packages via other mechanisms (e.g. during __init__ or Requires.jl too) for the same reason that they will sometimes cause it to deadlock or error unpredictably.

[KristofferC]

We could identify possible cycles from looking at the environment and warn/error on that?

It's already possible to create cycles in the dependency graph today (which will deadlock) so what is special about this case? Is it the factt that it depends on the order of the packages getting loaded and which package ends up triggering the actual loading of the extension?

[vtjnash]

Yes, with other cycles, it will always deadlock (and now error), which prevents people from doing it previously accidentally. For weakdeps though, now I think it can corrupt the compilecache directory, breaking Pkg.precompile(), and some users who load the package will see errors. What is subtly different here is that we release the package_locks just before loading weakdeps. This obscures that edge, so that it does not get correctly detected by the cycle detection. We did not do that before, since we would run any user code related to the package before dropping that lock / edge, but now we run weakdeps after dropping that lock.

[KristofferC]

I wonder if the problems that we've seen hasn't been from #48558. Are there any known reproducers of issues now with that fixed?

[vtjnash]

Yes, we know conclusively #48535 is required

[KristofferC]

Is there a concrete example that shows this and shows it is worse for extensions over normal packages? It would be good to have to e.g. create a test. The original motivation from Oscar seems to have been fully fixed AFAIU.

If moving the extension into the parent has the same cycle issue, I don't see this as being worse than the normal cycles one can currently create.

So (looking at #48513 (comment)) a cycle C -> A -> Bext -> C is the same as C -> A′ -> C where A′ = [A, Bext] (Bext moved from an extension into a submodule of A) which already gives you an issue.

[oscardssmith]

@KristofferC I think you are right. That said, I still think it is somewhat problematic that package extensions don't have a place to declare dependencies. It is very useful to make C -> A -> Bext -> C work, and it could if we knew ahead of time that Bext needs to load after C (without making Bext require C to trigger).