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

Allow listening on UNIX sockets for HTTP listeners #8103

Closed
wants to merge 1 commit into from

Conversation

auscompgeek
Copy link
Contributor

@auscompgeek auscompgeek commented Aug 17, 2020

Opening for preliminary review and CI run. Docs forthcoming after the sun comes back up at my end of the Earth, hopefully.

Closes: #4975

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.
  • Pull request includes a sign off
  • Code style is correct (run the linters)

Signed-off-by: David Vo <david@vovo.id.au>
@auscompgeek
Copy link
Contributor Author

This could probably do with a quick glance over now. I'll hopefully get docs written for this tomorrow.

A couple of open questions:

@clokep clokep requested a review from a team August 20, 2020 13:58
@richvdh
Copy link
Member

richvdh commented Aug 26, 2020

* https://github.com/matrix-org/synapse/pull/8103/files#diff-01aac508ef38061957e6fb961582e5ceR320
* Should the logging I'm adding be at ERROR?

Yes, I'd have thought it would be preferable to emit errors and bail than to carry on with warnings.

In general, it would be preferable to pick this sort of problem up during config parsing, which means that they will be reported before synapse forks (if it's run in daemonize mode) and can be reported by a config checker (cf #8160). Basically you can raise a ConfigError to get sensibly formatted output.

Some other thoughts:

  • it would be nice to add some type annotations for the new parameters, so that we know what a socket_options is.
  • could we have a generic _listen_socket function which takes a Union[TcpListenerConfig, UnixListenerConfig] and delegates to one of _listen_tcp, _listen_ssl, listenUNIX as appropriate? it seems like we have lots of copies of the same code, some of which seem unnecessarily restricted to not support unix socketts.

@richvdh richvdh removed the request for review from a team August 26, 2020 17:51
@richvdh richvdh added the X-Awaiting-Changes A contributed PR which needs changes and re-review before it can be merged label Aug 26, 2020
@clokep
Copy link
Contributor

clokep commented Oct 20, 2020

@auscompgeek Curious if you have plans to keep working on this?

@auscompgeek
Copy link
Contributor Author

Oops, sorry, I forgot this was on my queue…

This has simply been held back by my lack of time to actually work on this. I'd like to see this get across the line, but I'll be preoccupied for at least the next couple of weeks.

@erikjohnston
Copy link
Member

@auscompgeek I'm going to close this for now, but feel free to reopen if you get time to work on it some more!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
X-Awaiting-Changes A contributed PR which needs changes and re-review before it can be merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support UNIX sockets for HTTP interfaces
4 participants