zbraniecki
commented
3 years ago The final solution is based on the suggestion by @emilio to separate out an Rc<Bundles>> and let the user maintain its liveliness through the use case.
In perf tests against master, this model does not incure any perf hit and it does fix the bug!
This is a draft attempt to address the problem encountered when working on using
fluent-fallbackin Gecko.While all other oranges in taskcluster seem to be just races, there is one that is a crash - https://treeherder.mozilla.org/jobs?repo=try&revision=b8e6510cd9b750fbb6902e833df2b1c4a6edc5c1&selectedTaskRun=M_G5cJS4SieOKV6ehgN8tQ.0
The relevant part of the stack is:
The particular source line is https://hg.mozilla.org/try/file/3d5d1b1619ed5d6496e6b0dd2a40a07af5d01699/intl/l10n/rust/localization-ffi/src/lib.rs#l152
The cause of the crash seems to be that there is a call to
add_resource_idwhile an asyncformat_messagesis being executed.I am able to reproduce the crash locally if I apply the patchset and run the incriminated test with
--verify.I am not clear on how multi-threaded Gecko processes are and in what scenario a single document's Localization can be executing
format_messagesand in the middle calladd_resource_id.It may be there this is some flaw fixable with a switch from Rc to Arc, maybe? Or it may be that this is an inherent flaw in
fluent-fallback.In particular, the way we handle that in JS implementation is that we generate the cached iter/stream, and use it for all calls until the state is updated (state = sync + res_ids). When that happens, we regenerate the iter/stream and the next call to
format_messageswill use that, but the "old" ones that were in the middle when the state update happened will continue to use the old iter/stream.That seems to be really hard to reproduce in Rust because we went far into no-alloc land which means that the
format_messagesrelies onFluentBundlebeing allocated inLocalization's cache, and that, in turn, requires that the iter/stream remains available untilformat_messagesends.In this PR I did a poor solution to the problem - I keep the old iter/stream, and add a "new" one on top to a ChunkVec. This allows me to use the new one in the consequitive calls, while the old one remains available for calls that used it.
What I'm missing is some way of expressing that when nothing requires the old one anymore (all
format_messagecalls that relied on it completed), I'd like to drop it to avoid accumulating memory.I tried several approaches with Rc/Weak but that didn't work well with the
Cow's returned from the method.I'm not even sure if this is a sound approach, but I'd like to dump it here and ask for help in evaluating whether this is the right approach to resolve the issue.