Fix worker doc for synapse.app.frontend_proxy

[dklimpel]

Fixes: #3717

Pull Request Checklist

• Pull request is based on the develop branch
• Pull request includes a changelog file. The entry should:
  • Be a short description of your change which makes sense to users. "Fixed a bug that prevented receiving messages from other servers." instead of "Moved X method from EventStore to EventWorkerStore.".
  • Use markdown where necessary, mostly for code blocks.
  • End with either a period (.) or an exclamation mark (!).
  • Start with a capital letter.
  • Feel free to credit yourself, by adding a sentence "Contributed by @github_username." or "Contributed by [Your Name]." to the end of the entry.
• Pull request includes a sign off
• Code style is correct
  (run the linters)

Signed-off-by: Dirk Klimpel dirk@klimpel.org

[DMRobertson]

As of #6964, it looks like the frontend_proxy is really just a generic_worker in disguise. Should we move the frontend_proxy to the "Historical apps" section below?

(I'm very unfamiliar with the history here, maybe @matrix-org/synapse-core can advise).

[dklimpel]

There are more "Historical apps", IMO.

synapse/synapse/app/generic_worker.py

Lines 430 to 442 in 6dd7fa1

	assert config.worker.worker_app in (
	"synapse.app.appservice",
	"synapse.app.client_reader",
	"synapse.app.event_creator",
	"synapse.app.federation_reader",
	"synapse.app.federation_sender",
	"synapse.app.frontend_proxy",
	"synapse.app.generic_worker",
	"synapse.app.media_repository",
	"synapse.app.pusher",
	"synapse.app.synchrotron",
	"synapse.app.user_dir",
	)

• synapse.app.media_repository
• synapse.app.pusher
• synapse.app.federation_sender

[richvdh]

Yes indeed, there is absolutely no difference between a frontend_proxy worker and a generic_worker nowadays; we should just get rid of this section.

... which makes me realise that generic_worker can actually handle requests to ^/_matrix/client/(r0|v3|unstable)/keys/upload, and we should add it to the list up there.

Looks like ^/_matrix/client/(api/v1|r0|v3|unstable)/presence/ is already in the list.

[dklimpel]

I am not sure what I should do? IMO a redesign of the docs should not part of this PR.
Moving URLs to generic_worker has impact to docker image. Or deprecate three worker types is a bigger change.

[richvdh]

The only changes we need (for the documentation) are:

• remove the section on frontend_proxy
• add /_matrix/client/(r0|v3|unstable)/keys/upload to the list of URLs supported by generic_worker. It is currently supported, but not documented.

Moving URLs to generic_worker has impact to docker image.

we may need to update the docker image to match the recommendations from the documentation, but that can be done separately.

[dklimpel]

I do not see this in this PR. This is a bit more.

• frontend_proxy needs to be deprecated
• It is not a simple move/add /_matrix/client/(r0|v3|unstable)/keys/upload, it needs an extra paragraph for the config param worker_main_http_uri, what is needed for the URL

It would make sense to create a PR for depracte all of the redundant workers:

• synapse.app.media_repository
• synapse.app.pusher
• synapse.app.federation_sender

[richvdh]

I do not see this in this PR.

I can't insist, but it just doesn't seem worth making minor corrections to text that is fundamentally outdated.

This is a bit more.

• frontend_proxy needs to be deprecated

I don't think we need a particular deprecation notice. frontend_proxy is just another example of the redundant workers that are covered under Historical apps, much like client_reader and event_creator (which were removed in #7969).

• It is not a simple move/add /_matrix/client/(r0|v3|unstable)/keys/upload, it needs an extra paragraph for the config param worker_main_http_uri, what is needed for the URL

Oh bother, that's true. Yes, we should really move the existing text on worker_main_http_uri to the general Worker configuration section. (and probably mention it again next to /_matrix/client/(r0|v3|unstable)/keys/upload.

It would make sense to create a PR for depracte all of the redundant workers:

• synapse.app.media_repository
• synapse.app.pusher
• synapse.app.federation_sender

media_repository isn't (yet) redundant, afaik: a generic_worker cannot handle the /media endpoints. But in general I think it makes more sense to consider each worker app in turn, in separate PRs. #10441 tracks this work.

[DMRobertson]

Back in the queue for another set of eyes.

[richvdh]

Yes indeed, there is absolutely no difference between a frontend_proxy worker and a generic_worker nowadays; we should just get rid of this section.

... which makes me realise that generic_worker can actually handle requests to ^/_matrix/client/(r0|v3|unstable)/keys/upload, and we should add it to the list up there.

Looks like ^/_matrix/client/(api/v1|r0|v3|unstable)/presence/ is already in the list.

[richvdh]

It really doesn't matter whether use_presence is set to false or not: any worker can now handle ^/_matrix/client/.../presence/[^/]+/status. It is no longer a "stub".