synapse
synapse copied to clipboard
Silently drop PDUs we fail to parse from federation, instead of failing entire transaction
Related https://github.com/matrix-org/synapse/issues/7543 / #7543
Complement test: https://github.com/matrix-org/complement/pull/743
Sytest changes: https://github.com/matrix-org/sytest/pull/1391
Pull Request Checklist
- [x] Pull request is based on the develop branch
- [x] Pull request includes a changelog file.
- [x] Code style is correct
My reasoning for the error was that synapse would yell at you loudly before with a 500 for the whole transaction before, but there's not really anything another server can do with that information (a loud error)
Returning an error for that specific pdu at least lets the sending server output a warning or similar
From the discussion in #7543, i think the main problem is that most of the time it's impossible to calculate the event id, but here it was already provided. I'm not sure if there's a drawback to informing about the event not being processed
In the end i'm mostly worried about returning a 200 response every time though, regardless if it includes the error or not
~Err sorry, I obviously need more coffee. The exact opposite is true of what I said in #17893 (comment)~
Actually, it's partially true. This PR does two things, the latter of which the title fails to mention:
1. Silently drop PDUs we fail to parse from federation 2. Report an error back to the homeserver if an `event_id` was provided in an incoming v3+ room event.Discussion in #17893 (comment) is advocating to also silently drop events which contain an
event_idfield. So dropping these lines and removing the second change from this PR.@morguldir curious for your thoughts on that change?
I agree with @anoadragon453's assessment and think that we should merge this PR with these lines removed. This also means the test will need to be updated not to check for the error in the response.
Dropping the bad events is an important change that makes Synapse better, without being controversial. Keeping the potential per-event error responses, for a very specific case of event failures, can be decided on at a later point once things such as https://github.com/element-hq/synapse/issues/7543 & https://github.com/matrix-org/matrix-spec/issues/2027 get sorted out.