Manual reloading in version 2.1.12

[mocheryl]

With the latest version you can disable hot-reloading of configuration files, but this also prevents you from manually reloading them. Is this behavior intentional? I don’t think it should be as I don’t see any detrimental effects in having the possibility for manually reloading them always available.

[lifenjoiner]

How can manual reloading be possible for a console App?

[welwood08]

The wording implies manual reloading was at one point possible before v2.1.12, but I don't believe that to be the case.

Assuming instead that this is a request for a new feature, the usual way to trigger a background service to reload configuration on a UNIX-like system is via a signal such as SIGHUP or SIGUSR1/2. I'm not sure how cross-platform that is off the top of my head though...

[mocheryl]

The wording implies manual reloading was at one point possible before v2.1.12, but I don't believe that to be the case.

It's possible since 2.1.10. Please see setupSignalHandler in dnscrypt-proxy/hot_reload.go. In 2.1.12 however you can do it only with enable_hot_reload option set to true because otherwise the calling function InitHotReload exits before calling the signals listener.

[...] the usual way to trigger a background service to reload configuration on a UNIX-like system is via a signal such as SIGHUP or SIGUSR1/2. I'm not sure how cross-platform that is off the top of my head though

Exactly. I'm not sure about cross-platform either, but I use the existing manual reloading functionality in OpenBSD and it works just fine.

[lifenjoiner]

Letting enable_hot_reload only control whether set up the ConfigWatcher seems will work.

[mocheryl]

I think it should, but it doesn't, because the way it is set up now it also controls whether manual reloading is possible. So I still wonder if this is intentional, or just an oversight when making hot-reloading optional with the enable_hot_reload setting in the latest release.

[lifenjoiner]

I believe, when it was named hot-reloading, it was in contrast to restarting the app, which is usually called cold/hard/manual operation.
Forcibly sending a SIGHUP in, is a special case. I'd like to treat it as the 3rd case, in the middle and platform limited. It is not covered.

[welwood08]

The wording implies manual reloading was at one point possible before v2.1.12, but I don't believe that to be the case.

It's possible since 2.1.10. Please see setupSignalHandler in dnscrypt-proxy/hot_reload.go. In 2.1.12 however you can do it only with enable_hot_reload option set to true because otherwise the calling function InitHotReload exits before calling the signals listener.

[...] the usual way to trigger a background service to reload configuration on a UNIX-like system is via a signal such as SIGHUP or SIGUSR1/2. I'm not sure how cross-platform that is off the top of my head though

Exactly. I'm not sure about cross-platform either, but I use the existing manual reloading functionality in OpenBSD and it works just fine.

Apologies for my mistake, I had not examined the new code fully.

I'm not currently in a position to test code changes, but it appears safe to move

dnscrypt-proxy/dnscrypt-proxy/hot_reload.go

Lines 103 to 104 in 2d099e2

	// Setup SIGHUP handler for manual reload
	setupSignalHandler(proxy, plugins)

to just after

dnscrypt-proxy/dnscrypt-proxy/hot_reload.go

Line 41 in 2d099e2

proxy.pluginsGlobals.RUnlock()

because then plugins is populated. Then

dnscrypt-proxy/dnscrypt-proxy/hot_reload.go

Lines 13 to 22 in 2d099e2

	// Check if hot reload is enabled
	if !proxy.enableHotReload {
	dlog.Notice("Hot reload is disabled")
	return nil
	}
	dlog.Notice("Hot reload is enabled")
	// Create a new configuration watcher
	configWatcher := NewConfigWatcher(1000) // Check every second

could be moved below that immediately before

dnscrypt-proxy/dnscrypt-proxy/hot_reload.go

Line 43 in 2d099e2

// Register plugins for config watching

so that manual reloading is supported even while hot reload is disabled. I could probably make this a PR if someone else is able to test?

[lifenjoiner]

How about changing enable_hot_reload to a 3-state option? People on platforms which can't send SIGHUP in, can disable it entirely, to save some resources or avoid being attacked by SIGHUP flood.

[welwood08]

Is SIGHUP flood a thing? If an attacker has appropriate permissions on the system to send IPC signals to the dnscrypt-proxy process, they have permissions to do many other things too. Plus the default handling of a SIGHUP without an event handler is to terminate the process, so not registering a handler is hardly a safety net from such an attack.

Saving resources is commendable, though I have concerns about changing a boolean config value to another type for already released versions and the amount of work that pushes to every user to update their configs. Is the amount of resources saved great enough to offset that?

[lifenjoiner]

I don't know much about these platforms. Just mentioning it.

[mocheryl]

I could probably make this a PR if someone else is able to test?

Sure! I'm already testing the latest version at the moment so I can continue to perform some more tests.
Thanks for your consideration!