refactor: Provide API to stop listener programatically

[mkleczek]

DISCLAIMER:
This commit was authored entirely by a human without the assistance of LLMs.

This change introduces possibility to stop listener thread programatically. It is a prerequisite for HSpec tests starting the listener as they have to be able to stop it when cleaning up.

[wolfgangwalther]

I'm not a fan of exposing internal APIs just for tests - that means we're shipping code that we don't actually need.

Is it a requirement to test this from hspec? Can't we observe the same thing in an IO test?

[mkleczek]

I'm not a fan of exposing internal APIs just for tests - that means we're shipping code that we don't actually need.

Me neither but in this particular case, I'd say there is more to it. The direct reason for this change is to be able to test from hspec but, to be honest, the way how we handle closing the listener at the moment begs for improvement - today we just kill the thread not letting it do the cleanup.
This change makes it possible to gracefully trigger listener shutdown (it is not implemented in this PR though).

Is it a requirement to test this from hspec? Can't we observe the same thing in an IO test?

This is a longer discussion started here: #4668 (comment)
Based on it we merged #4732 and #4671 to facilitate implementing more tests in Haskell. Continuing in this direction we have #4836 pending in the queue.

Testing in Haskell has several advantages but the most important thing is typed observations monitoring (contrary to io tests where we parse log messages which is extremely brittle).

Answering directly: no, there is no requirement to test from Hspec but it is much more convenient. There are more listener related Hspec tests in the queue in #4673 (eg. https://github.com/mkleczek/postgrest/blob/b8ec90af233ff3d377426e1b23b7e1db2c989b29/test/observability/Observation/ToxiSpec.hs#L66) - waiting for #4836 to be merged.

[steve-chavez]

I'm not a fan of exposing internal APIs just for tests - that means we're shipping code that we don't actually need.

Note: we're already doing this for the PGRST_INTERNAL_* sleeps.

postgrest/src/PostgREST/SchemaCache.hs

Lines 171 to 173 in 127490e

	_ <-
	let sleepCall = SQL.Statement "select pg_sleep($1 / 1000.0)" (param HE.int4) HD.noResult True in
	for_ configInternalSCQuerySleep (`SQL.statement` sleepCall) -- only used for testing

I've been thinking, couldn't we do these points of failure/testing based on Observations? These are essentially callbacks that can execute IO (), they can be kinda like PostgreSQL injection points.

That would allow us to not ship the testing code in the binaries, we would only inject the sleeps (and other things) during testing via a callback.

[wolfgangwalther]

This change introduces possibility to stop listener thread programatically. It is a prerequisite for HSpec tests starting the listener as they have to be able to stop it when cleaning up.

I looked at this from a perspective of "what would I do from non-hspec tests, for example pytest, to start/stop the listener?". It seems like db-channel-enabled is reloadable, so I should be able to change the config file and reload to do just that.

[mkleczek]

It seems like db-channel-enabled is reloadable

I believe it is not - there is no code that would stop/start listener thread upon config reload. If it is marked as such in the docs it is a documentation issue.

[wolfgangwalther]

It seems like db-channel-enabled is reloadable

I believe it is not - there is no code that would stop/start listener thread upon config reload. If it is marked as such in the docs it is a documentation issue.

Interesting, I was going by the docs only. Maybe we can fix the code instead of the documentation then, to make it reloadable?

[steve-chavez]

I believe it is not - there is no code that would stop/start listener thread upon config reload. If it is marked as such in the docs it is a documentation issue.

Yeah it's a docs issue. I remember deciding against it because if we allow users to disable the LISTEN in-db then they:

• Break the auto reload event trigger.
• Can't bring the LISTEN back up with the in-db config + a NOTIFY (obviously). They would need to do a SIGUSR2 (not available on all environments) or restart the service (downtime) to reload the config.

So I don't think we should allow disabling the LISTEN via in-db config. Thoughts?

[wolfgangwalther]

This was not about in-db, but just about basic reloading. The docs say:

Reloadable: Y
In-Database: n/a

I believe we should implement it exactly like this, if that's not already the case.

[steve-chavez]

Right, my bad, got confused.

I believe we should implement it exactly like this, if that's not already the case.

Agree in making it reloadable by unix signal.

Opened #4894

[steve-chavez]

@mkleczek Shouldn't we prefer to do #4894 instead of this PR? Or do you think #4894 will be insufficient?

[mkleczek]

@mkleczek Shouldn't we prefer to do #4894 instead of this PR? Or do you think #4894 will be insufficient?

To implement #4894 we need to implement something like this PR anyway (because we have to be able to stop/start listener from the unix signal handler).

[wolfgangwalther]

To implement #4894 we need to implement something like this PR anyway (because we have to be able to stop/start listener from the unix signal handler).

Then let's do it here, if we already have the basic pieces in place. I think we should:

• Make it reloadable here.
• Not write a test that makes this stop/start listener logic some API that can/should be used from tests (I personally don't agree with the direction, but we should discuss this in the place where you'd write the first actual test to make use of it this way, not here)
• Implement a regular IO test to confirm that db-channel-enabled is indeed reloadable.

[steve-chavez]

@mkleczek Why was this closed?