Skip to content
This repository was archived by the owner on Apr 26, 2024. It is now read-only.

Improved validation for received requests #9817

Merged
merged 4 commits into from
Apr 23, 2021
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
Simplify start_listening callpath
There's really no point in having `hs.start` pull out the listener config, so
that it can be passed down into `_base.start`, and then back into
`hs.start_listening`.
  • Loading branch information
richvdh committed Apr 23, 2021
commit 1cdb1308d886315babd69b58e15301a26909343e
6 changes: 2 additions & 4 deletions synapse/app/_base.py
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,6 @@
import synapse
from synapse.app import check_bind_error
from synapse.app.phone_stats_home import start_phone_stats_home
from synapse.config.server import ListenerConfig
from synapse.crypto import context_factory
from synapse.logging.context import PreserveLoggingContext
from synapse.metrics.background_process_metrics import wrap_as_background_process
Expand Down Expand Up @@ -288,7 +287,7 @@ def refresh_certificate(hs):
logger.info("Context factories updated.")


async def start(hs: "synapse.server.HomeServer", listeners: Iterable[ListenerConfig]):
async def start(hs: "synapse.server.HomeServer"):
"""
Start a Synapse server or worker.

Expand All @@ -300,7 +299,6 @@ async def start(hs: "synapse.server.HomeServer", listeners: Iterable[ListenerCon

Args:
hs: homeserver instance
listeners: Listener configuration ('listeners' in homeserver.yaml)
"""
# Set up the SIGHUP machinery.
if hasattr(signal, "SIGHUP"):
Expand Down Expand Up @@ -336,7 +334,7 @@ def run_sighup(*args, **kwargs):
synapse.logging.opentracing.init_tracer(hs) # type: ignore[attr-defined] # noqa

# It is now safe to start your Synapse.
hs.start_listening(listeners)
hs.start_listening()
hs.get_datastore().db_pool.start_profiling()
hs.get_pusherpool().start()

Expand Down
8 changes: 1 addition & 7 deletions synapse/app/admin_cmd.py
Original file line number Diff line number Diff line change
Expand Up @@ -70,12 +70,6 @@ class AdminCmdSlavedStore(
class AdminCmdServer(HomeServer):
DATASTORE_CLASS = AdminCmdSlavedStore

def _listen_http(self, listener_config):
pass

def start_listening(self, listeners):
pass


async def export_data_command(hs, args):
"""Export data for a user.
Expand Down Expand Up @@ -232,7 +226,7 @@ def start(config_options):

async def run():
with LoggingContext("command"):
_base.start(ss, [])
_base.start(ss)
await args.func(ss, args)

_base.start_worker_reactor(
Expand Down
8 changes: 4 additions & 4 deletions synapse/app/generic_worker.py
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@
# limitations under the License.
import logging
import sys
from typing import Dict, Iterable, Optional
from typing import Dict, Optional

from twisted.internet import address
from twisted.web.resource import IResource
Expand Down Expand Up @@ -374,8 +374,8 @@ def _listen_http(self, listener_config: ListenerConfig):

logger.info("Synapse worker now listening on port %d", port)

def start_listening(self, listeners: Iterable[ListenerConfig]):
for listener in listeners:
def start_listening(self):
for listener in self.config.worker_listeners:
if listener.type == "http":
self._listen_http(listener)
elif listener.type == "manhole":
Expand Down Expand Up @@ -468,7 +468,7 @@ def start(config_options):
# streams. Will no-op if no streams can be written to by this worker.
hs.get_replication_streamer()

register_start(_base.start, hs, config.worker_listeners)
register_start(_base.start, hs)

_base.start_worker_reactor("synapse-generic-worker", config)

Expand Down
8 changes: 4 additions & 4 deletions synapse/app/homeserver.py
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@
import logging
import os
import sys
from typing import Iterable, Iterator
from typing import Iterator

from twisted.internet import reactor
from twisted.web.resource import EncodingResourceWrapper, IResource
Expand Down Expand Up @@ -268,14 +268,14 @@ def _configure_named_resource(self, name, compress=False):

return resources

def start_listening(self, listeners: Iterable[ListenerConfig]):
def start_listening(self):
if self.config.redis_enabled:
# If redis is enabled we connect via the replication command handler
# in the same way as the workers (since we're effectively a client
# rather than a server).
self.get_tcp_replication().start_replication(self)

for listener in listeners:
for listener in self.config.server.listeners:
if listener.type == "http":
self._listening_services.extend(
self._listener_http(self.config, listener)
Expand Down Expand Up @@ -407,7 +407,7 @@ async def start():
# Loading the provider metadata also ensures the provider config is valid.
await oidc.load_metadata()

await _base.start(hs, config.listeners)
await _base.start(hs)

hs.get_datastore().db_pool.updates.start_doing_background_updates()

Expand Down
8 changes: 8 additions & 0 deletions synapse/server.py
Original file line number Diff line number Diff line change
Expand Up @@ -287,6 +287,14 @@ def setup(self) -> None:
if self.config.run_background_tasks:
self.setup_background_tasks()

def start_listening(self) -> None:
"""Start the HTTP, manhole, metrics, etc listeners

Does nothing in this base class; overridden in derived classes to start the
appropriate listeners.
"""
pass
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I find it a bit odd that the default implementation is a no-op, given the doc comment. What do you think about making this an abstract method and keeping the no-op version in admin cmd?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

personally, it seems fairly reasonable that the default be a no-op - I basically see it as a hook that the derived classes can override. I could make it clearer in the docstring?

don't feel that strongly though, so let me know what you think.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess it just feels slightly odd that start_listening() doesn't do the right thing by default (which it feels like it could if we deduplicate the code between master and worker apps), the fact that the admin cmd server is no-oping it is a special thing that its doing that I'd expect to be specifically called out in its definition.

It doesn't really matter though, given we're not really adding new server objects.


def setup_background_tasks(self) -> None:
"""
Some handlers have side effects on instantiation (like registering
Expand Down