This is not trivial to fix. Though making CodeContext weak may appear to be safe, it might be not. Think of an application using libvala that creates the CodeContex and forgets it. It now runs, but changing to weak may lead to crashes.
On the other hand, making resolver/analyzer/etc weak is not suitable either.

For now it's better to keep things as is, and let the application nullify resolver/analyzer/etc by hand to avoid leaks.

So the recommended way to do this is not to use context.check(), but instead construct your own instances of resolver/analyzer instead?

context.check() is fine, it's only that we don't know if existing applications rely on the owned references.

However, note that if you use context.check() then you need to rely on some hoops to make sure the CodeContext will properly release. What I currently do is this to make that happen:

context.resolver.resolve(new CodeContext());
context.analyzer.context = null;
context.flow_analyzer.analyze(new CodeContext());

Which is a pretty non-obvious thing to do. Also, I explicitly need to set context.analyzer to null, instead of trying to analyze an empty CodeContext since that will spew a lot of null warnings.

Created attachment 346523 [details] [review]
Fix for bug 712694

Vala resolver, analyzer, flow_analyzer and used_attr, requires a context just in to check, analyze or resolve, then at the end of each method, this patch set context to null, this is save if context is used to start check, analyze and resolve.

This should resolve lot of segfaults at compile time, because resources not released.

@daniel: So your 6 patches boil down to adding "this.context = null;" in three places. Those look absolutely valid, but I don't see how this fixes segfaults. Although it might introduce null-references as a side-effect.

Well may is too early to say that these patches will prevent crashes. At GXml compilation time I saw valac crashes all the time but alleatory. I saw same crashes in GNOME Builder either.


So while is true a bug, as explained by reporter, this will make to have un released resources, even by valac itself making possible to fall in crashes due to  lack of resources.

Then we need to deprecate context properties for analyzer, resolver and checks, because they can be created at check() call in context, allowing to unref them when finished.

We need to deprecate analyzers context property and advice users that this property will be null all the time, except when an analysis or check is in progress.

In long term, all these objects should work alone, to run independent each other, forcing users to keep their own reference of the object.

To accomplish above, we need to remove context's properties for analyzers and checkers, to avoid circular references.

Created attachment 365527 [details] [review]
vala: Use CodeContext.root instead of SemanticAnalyzer.root_symbol

Created attachment 365528 [details] [review]
Release taken references to CodeContext after using it

These patches have been pushed upstream, so they resolve the issue described here. Closing it. If a redesign/change is required a new bug should be opened.

https://git.gnome.org/browse/vala/commit/?id=2dc47785171f8f62f7f017939681e0d5cf3e9847


https://git.gnome.org/browse/vala/commit/?id=0c05b6e39d2a1f10cdd3f155c85d39f7a563080b

I left this bug open on purpose while I am not sure this is all needed to be fixed.

@ben: Can you share your experience regarding https://github.com/benwaffle/vala-language-server/blob/master/vala038_workarounds.vala ?

Princeton wrote that code

(In reply to Rico Tzschichholz from comment #12)
> @ben: Can you share your experience regarding
> https://github.com/benwaffle/vala-language-server/blob/master/
> vala038_workarounds.vala ?

(In reply to Ben from comment #13)
> Princeton wrote that code

That "solution" I came to partially by inspection of libvala's code, and partially by experimenting. Some of that code might not be strictly necessary (perhaps the stuff that "breaks" Namespace references).

By inspection I found the three major circular references between CodeContext's and resolver/analyzer/flow_analyzer. (see https://github.com/benwaffle/vala-language-server/blob/master/vala038_workarounds.vala#L69) I also noticed that each Vala.Symbol has a strong ref to a Vala.Scope, which holds a container (Map<string,Symbol>) of strong refs to Symbols. So I clear the symbol table of each Scope just to be sure.

Without this fix, each time the language server compiles on just a single keyboard press, an extra 80-140 MiB (!) is leaked. With this fix, typically around 5 MiB.

I also noticed there are some members that are private (like Scope.anonymous_members), that might be candidates for circular refs.

-- GitLab Migration Automatic Message --

This bug has been migrated to GNOME's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.gnome.org/GNOME/vala/issues/418.