Page MenuHomePhabricator

Conpherence - fix a fatal
ClosedPublic

Authored by btrahan on Apr 9 2015, 4:46 PM.

Details

Summary

Ref T7795.

I can't get this to reproduce and its confusing to me how its possible. The trace in T7795 uses the "LOAD" pathway on the update controller. Under the hood, this issues a ThreadQuery with needTransactions to true. With needTransactions to true, the transactions and pertinent handles are all loaded nicely.

So... best guess is there has been some LIMIT of transactions since the offending person participated...? Alternative fix which would probably work is to specify needParticipantCache to true.

More on T7795 - the user report found the "a, b, c..." subtitle thing in the messages dropdown confusing. Yet another fix here would be to change that to be something like "a: snippet of what a said...". I'll discuss that on the task.

Test Plan

iiam

Diff Detail

Repository
rP Phabricator
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

btrahan retitled this revision from to Conpherence - fix a fatal.
btrahan updated this object.
btrahan edited the test plan for this revision. (Show Details)
btrahan added a reviewer: epriestley.
epriestley edited edge metadata.

In loadAndRenderUpdates(), we always seem to end up in getDisplayData(), but only the ConpherenceUpdateActions::METADATA pathway actually loads the participant cache.

If one of the recent participants has not said anything in the last 100 transactions, it seems like the other pathways should always break? That is, I'd expect something like this:

  • A and B join.
  • A says something.
  • B says something.
  • A says 105 things.

...to cause issues?

At some point, we can probably simplify this by using the newer Handle logic (which generally requires less up-front loading). This fix doesn't feel great but obviously won't hurt anything...

This revision is now accepted and ready to land.Apr 9 2015, 5:03 PM

Yeah, I'll probably hold this until T7795 product discussion is figured out and then fix this based on that.

I updated to HEAD and then applied this patch to the test server I created which had a repro based on T7795#106182, and this does appear to fix the server 500 error

btrahan edited edge metadata.

Fix the data loading directly

  • always load participant cache
  • change the query code slightly so you can load participant cache and participants (changed "or" to "and")
  • stop loading participant cache up front and only load participants in the narrow case we need it for settings
This revision was automatically updated to reflect the committed changes.

@nevogd - thanks for the testing. ended up fixing this in a slightly different way but if you update to HEAD it should still be resolved.