Fix exceptions in logs when failing to get remote room list

[erikjohnston]

Fixes exceptions:

TimeoutError: User timeout caused connection failure.
  File "synapse/http/matrixfederationclient.py", line 575, in _send_request
    response = await request_deferred
  File "twisted/internet/defer.py", line 1658, in _inlineCallbacks
    cast(Failure, result).throwExceptionIntoGenerator, gen
  File "twisted/python/failure.py", line 500, in throwExceptionIntoGenerator
    return g.throw(self.type, self.value, self.tb)
  File "synapse/http/federation/matrix_federation_agent.py", line 190, in request
    self._agent.request(method, uri, request_headers, bodyProducer)
  File "twisted/internet/defer.py", line 1658, in _inlineCallbacks
    cast(Failure, result).throwExceptionIntoGenerator, gen
  File "twisted/python/failure.py", line 500, in throwExceptionIntoGenerator
    return g.throw(self.type, self.value, self.tb)
  File "synapse/http/federation/matrix_federation_agent.py", line 295, in _do_connect
    raise first_exception
  File "synapse/http/federation/matrix_federation_agent.py", line 282, in _do_connect
    endpoint.connect(protocol_factory)
RequestSendFailed: Failed to send request: TimeoutError: User timeout caused connection failure.
  File "twisted/internet/defer.py", line 1658, in _inlineCallbacks
    cast(Failure, result).throwExceptionIntoGenerator, gen
  File "twisted/python/failure.py", line 500, in throwExceptionIntoGenerator
    return g.throw(self.type, self.value, self.tb)
  File "synapse/federation/federation_client.py", line 1122, in get_public_rooms
    third_party_instance_id=third_party_instance_id,
  File "synapse/federation/transport/client.py", line 451, in get_public_rooms
    destination=remote_server, path=path, args=args, ignore_backoff=True
  File "synapse/http/matrixfederationclient.py", line 1013, in get_json
    timeout=timeout,
  File "synapse/http/matrixfederationclient.py", line 391, in _send_request_with_optional_trailing_slash
    response = await self._send_request(request, **send_request_args)
  File "synapse/http/matrixfederationclient.py", line 579, in _send_request
    raise RequestSendFailed(e, can_retry=True) from e

Sentry: https://sentry.matrix.org/sentry/synapse-matrixorg/issues/226330

[babolivier]

Don't we want to log the error as well?

[erikjohnston]

We'll have already logged that the federation request has failed, so I don't think there's much point

[richvdh]

I'm failing to grok how this solves the problem. More context pls?

(also, why does sentry show this problem starting 13 days ago? what changed?)

[richvdh]

I don't think we should be picking error codes here: that should be handled in the REST server code, not the federation client code.

[erikjohnston]

Yeees, though what are you envisaging exactly? We can add an exception type for CouldNotTalkToRemote, but would that extend SynapseError (which is what we usually do), or handled explicitly at the rest layer, or the servlet layer?

Or would it be easier to hoist this back up into the room list handler again?

[richvdh]

Right. I notice that get_remote_public_room_list has yet another layer of exception handling:

synapse/synapse/handlers/room_list.py

Lines 378 to 390 in 4f65b5d

	except HttpResponseException as hre:
	syn_err = hre.to_synapse_error()
	if hre.code in (404, 405) or syn_err.errcode in (
	Codes.UNRECOGNIZED,
	Codes.NOT_FOUND,
	):
	logger.debug("Falling back to locally-filtered /publicRooms")
	else:
	# Not an error that should trigger a fallback.
	raise SynapseError(502, "Failed to fetch room list")
	except RequestSendFailed:
	# Not an error that should trigger a fallback.
	raise SynapseError(502, "Failed to fetch room list")

. Indeed, it looks like #10400 might have messed this up.

And, if that weren't enough of a mess, PublicRoomListRestServlet has another round of catching HttpResponseException, which I think is meant to be unreachable, but who the hell knows.

I think you should allow the RequestSendFailed / HttpResponseExceptions to propagate into RoomListHandler and raise an appropriate SynapseError there; and remove the handling from PublicRoomListRestServlet. And, for the love of $deity, document the behaviour of these things.

[erikjohnston]

Wheeeeeee untested features

[erikjohnston]

I'm failing to grok how this solves the problem. More context pls?

The issue is that in the room list handler method we actually call get_public_rooms twice, and we'd only wrapped one of them in try/except. It seemed easier to move the error handling into the called function.

synapse/synapse/handlers/room_list.py

Lines 426 to 454 in 4f65b5d

	repl_layer = self.hs.get_federation_client()
	if search_filter:
	# We can't cache when asking for search
	return await repl_layer.get_public_rooms(
	server_name,
	limit=limit,
	since_token=since_token,
	search_filter=search_filter,
	include_all_networks=include_all_networks,
	third_party_instance_id=third_party_instance_id,
	)
	key = (
	server_name,
	limit,
	since_token,
	include_all_networks,
	third_party_instance_id,
	)
	return await self.remote_response_cache.wrap(
	key,
	repl_layer.get_public_rooms,
	server_name,
	limit=limit,
	since_token=since_token,
	search_filter=search_filter,
	include_all_networks=include_all_networks,
	third_party_instance_id=third_party_instance_id,
	)

(also, why does sentry show this problem starting 13 days ago? what changed?)

There are a bunch of unlinked sentry issues around roughly the same sort of code, so might be that. Or the fact I fixed this for one case previously so the stack traces are probably different.

[richvdh]

lgtm

[richvdh]

I've got to wonder if caching a RequestSendFailed is the right thing to do, but that's a separate problem. 🤷‍♂️

[erikjohnston]

🤷‍♂️