search

matrix-org / sygnal

Sygnal: reference Push Gateway for Matrix
Apache License 2.0
155 stars 140 forks source link

Does not work with Conduit #295

Closed timokoesters closed 2 years ago

timokoesters commented 2 years ago

Conduit sometimes sends pushes with "default_payload":null

Sygnal crashes like this:

Feb 09 17:09:43 corus.matrix.org sygnal-main[19588]: Traceback (most recent call last):
Feb 09 17:09:43 corus.matrix.org sygnal-main[19588]:   File "/opt/sygnal/env/lib/python3.7/site-packages/sygnal/http.py", line 275, in _handle_dispatch
Feb 09 17:09:43 corus.matrix.org sygnal-main[19588]:     result = await pushkin.dispatch_notification(notif, d, context)
Feb 09 17:09:43 corus.matrix.org sygnal-main[19588]:   File "/opt/sygnal/env/lib/python3.7/site-packages/sygnal/notifications.py", line 217, in dispatch_notification
Feb 09 17:09:43 corus.matrix.org sygnal-main[19588]:     return await self._dispatch_notification_unlimited(n, device, context)
Feb 09 17:09:43 corus.matrix.org sygnal-main[19588]:   File "/opt/sygnal/env/lib/python3.7/site-packages/sygnal/gcmpushkin.py", line 350, in _dispatch_notification_unlimited
Feb 09 17:09:43 corus.matrix.org sygnal-main[19588]:     data = GcmPushkin._build_data(n, device)
Feb 09 17:09:43 corus.matrix.org sygnal-main[19588]:   File "/opt/sygnal/env/lib/python3.7/site-packages/sygnal/gcmpushkin.py", line 426, in _build_data
Feb 09 17:09:43 corus.matrix.org sygnal-main[19588]:     data.update(device.data.get("default_payload", {}))
Feb 09 17:09:43 corus.matrix.org sygnal-main[19588]: TypeError: 'NoneType' object is not iterable

I plan to avoid sending nulls with the next Conduit release, but it would help if you add a quick workaround to Sygnal and deploy it.

timokoesters commented 2 years ago

I saw that something related to this was done here: https://github.com/matrix-org/sygnal/commit/80f1d4820f248730cff626974496c0960b0d010c But this still rejects the pushes.

clokep commented 2 years ago

There was some discussion around https://github.com/matrix-org/sygnal/pull/292#discussion_r779757254 about whether it should be ignored or rejected.

timokoesters commented 2 years ago

Note that I saw Conduit not working on the current matrix.org Sygnal instance, so #292 did not break it, it was already broken.

richvdh commented 2 years ago

I saw that something related to this was done here: 80f1d48 But this still rejects the pushes.

Note that this change only changed sygnal's behaviour to return a 400 rather than a 500 in this situation.

As this isn't a recent regression, we're reluctant to add in workarounds that run the risk of becoming permanent. Sorry!

squahtx commented 2 years ago

Looks like "default_payload": null was coming from this code that got fixed: https://github.com/ruma/ruma/commit/932fe4fa38accee577d1ab09ad004fb68e84d1d9