New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix exceptions when handling incoming transactions #3968

Merged
merged 6 commits into from Oct 1, 2018

Conversation

Projects
None yet
2 participants
@richvdh
Member

richvdh commented Sep 26, 2018

Fixes a number of problems in on_receive_pdu - in particular #3923 and #3934. Suggest looking at the commits for more information.

This PR builds on #3967 and #3959.

richvdh added a commit to matrix-org/sytest that referenced this pull request Sep 26, 2018

Tests for missing prevs in incoming transactions
This tries to test the things that are fixed in
matrix-org/synapse#3968.

richvdh added some commits Sep 26, 2018

Fix "unhashable type: 'list'" exception in federation handling
get_state_groups returns a map from state_group_id to a list of FrozenEvents,
so was very much the wrong thing to be putting as one of the entries in the
list passed to resolve_events_with_factory (which expects maps from
(event_type, state_key) to event id).

We actually want get_state_groups_ids().values() rather than
get_state_groups().

This fixes the main problem in #3923, but there are other problems with this
bit of code which get discovered once you do so.
Include state from remote servers in pdu handling
If we've fetched state events from remote servers in order to resolve the state
for a new event, we need to actually pass those events into
resolve_events_with_factory (so that it can do the state res) and then persist
the ones we need - otherwise other bits of the codebase get confused about why
we have state groups pointing to non-existent events.
Include event when resolving state for missing prevs
If we have a forward extremity for a room as `E`, and you receive `A`, `B`,
s.t. `A -> B -> E`, and `B` also points to an unknown event `X`, then we need
to do state res between `X` and `E`.

When that happens, we need to make sure we include `X` in the state that goes
into the state res alg.

Fixes #3934.

richvdh added a commit to matrix-org/sytest that referenced this pull request Sep 27, 2018

Tests for missing prevs in incoming transactions
This tries to test the things that are fixed in
matrix-org/synapse#3968.

richvdh added some commits Sep 27, 2018

Remove redundant, failing, test
This test didn't do what it claimed to do, and what it claimed to do was the
same as test_cant_hide_direct_ancestors anyway.

This stuff is tested by sytest anyway.

@richvdh richvdh requested a review from matrix-org/synapse-core Sep 27, 2018

@richvdh richvdh merged commit b5b93f4 into develop Oct 1, 2018

6 checks passed

ci/circleci: sytestpy2merged Your tests passed on CircleCI!
Details
ci/circleci: sytestpy2postgresmerged Your tests passed on CircleCI!
Details
ci/circleci: sytestpy3merged Your tests passed on CircleCI!
Details
ci/circleci: sytestpy3postgresmerged Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@richvdh richvdh deleted the rav/fix_federation_errors branch Oct 1, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment