Display an actual TCP port app is bound to #3034

develop7 · 2023-11-01T13:28:58Z

Documents and properly handles the special value of 0 for server-port config. Before this PR, the value was passed through and handled by the platform's getaddrinfo implementation, seemingly resulting in binding to a random port. This PR makes it official and binds to random port explicitly.

Other changes it introduces:

Simplifies app starting logic, moving platform-dependent POSIX-specific code to PostgREST.Unix, making it the only file with the LANGUAGE CPP pragma and reducing the amount of indirection. Pretty sure the indirection in question was here on purpose, let me know if that's still the case — tests do pass, things work fine, so I've decided to give it a shot.
Recent releases of network and all Windows released since 5 years ago actually do support Unix domain sockets, so I've taken a chance to prepare code for making use of those: replaced setFileMode with its twin function from unix-compat and used Network.Socket.isUnixDomainSocketAvailable to check such sockets' availability in runtime. All that's left to pull the plug is to upgrade network.
makes PostgREST.Unix.installSignalHandlers more explicit, avoiding potential circular dependency between PostgREST.Unix and PostgREST.AppState
slightly decreases test coverage. The cases missed seem to be trivial to me and would require considerable effort to be tested properly.

Fixes #2420

Old TODO

TODO:

test
document special server-port value of 0; done in server-port can be 0 postgrest-docs#695
get runtime error for Unix socket on unsupported platforms back (right now it degraded to compile error)
fix Windows build
~~fix the crash in, presumably, Admin.reachMainApp~~ I was binding a socket, but didn't listen it 😖

steve-chavez · 2023-11-01T19:20:37Z

@develop7 Good initiative fixing this!

I see the CI failures. That happens because the Admin.hs doesn't keep track of the assigned random port.

I believe the correct fix is the one that Wolfgang mentioned on #2174 (comment). Basically store the socket on AppState and use it on Admin.hs. Then some code can be removed from Admin, like the getAddrInfo part.

(I don't recall why I didn't make like that on the first place, but seems it should work)

develop7 · 2023-11-02T15:33:52Z

I believe the correct fix is the one that Wolfgang mentioned on #2174 (comment). Basically store the socket on AppState and use it on Admin.hs. Then some code can be removed from Admin, like the getAddrInfo part.

Yep, on it; the 47df6c2 is more of a PoC

develop7 · 2023-11-02T15:59:47Z

BTW since Warp.runSettings passes the port number through, it should handle special values like 0 properly as well, by returning actual port number.

develop7 · 2023-11-02T16:06:59Z

So Warp.runSettings does pass the port number to streaming-commons' bindPortTCP, which has sister function bindRandomPortTCP, which returns port actually bound; we could use it to handle the special case of 0 on our side. Let me explore this for a bit.

develop7 · 2023-11-03T16:46:24Z

Not sure whether it's better to store port number along in the AppState, along with app socket, or read port number directly out of socket. I chose the latter for the places where actual Warp instances are run, but initSockets is currently a bit inconsistent on this matter

steve-chavez · 2023-11-05T00:22:23Z

@develop7 It's starting to look better! Seems you need to add streaming-commons to cabal to clear the errors on CI.

Not sure whether it's better to store port number along in the AppState, along with app socket, or read port number directly out of socket. I chose the latter..

Yeah, the less state we can keep the better.

develop7 · 2023-11-06T15:23:39Z

@steve-chavez done and done, but now every single IO test under test_io.py is failing (see https://github.com/PostgREST/postgrest/actions/runs/6772230579/job/18404342720?pr=3034#step:4:269). It seems to timeout getting the 200 response from admin interface.

src/PostgREST/Unix.hs

develop7 · 2023-11-09T16:05:40Z

Okay, I've just reproduced the crash in Admin.reachMainApp. It's probably caused by reusing the Socket from AppState to send to it; one shouldn't do that.

develop7 · 2023-11-09T16:25:49Z

https://hackage.haskell.org/package/network-3.1.4.0/docs/Network-Socket.html#v:isUnixDomainSocketAvailable says

AF_UNIX is supported on Windows since 3.1.3.0. So, this variable is True on all platforms.

Does it mean we can have UNIX sockets on Windows?

develop7 · 2023-11-09T16:28:57Z

Does it mean we can have UNIX sockets on Windows?

Per haskell/network#549 (comment): any supported version of Windows has it. It's a "yes".

steve-chavez · 2023-11-17T17:49:30Z

postgrest.cabal

-      , directory >= 1.2.6 && < 1.4
-    exposed-modules:
-      PostgREST.Unix


@develop7 Any concerns on windows? Maybe we should allow running tests on windows on CI to be extra sure (can be done on another PR).

That'd be only fair, since Windows got first-class support

We cannot enable all tests on Windows right off the bat, but specs & doctests should be fine

postgrest.cabal

steve-chavez · 2023-11-22T17:51:40Z

postgrest.cabal

                    , swagger2                  >= 2.4 && < 2.9
                    , text                      >= 1.2.2 && < 1.3
                    , time                      >= 1.6 && < 1.12
                    , timeit                    >= 2.0 && < 2.1
                    , unordered-containers      >= 0.2.8 && < 0.3
+                    , unix-compat >= 0.6 && < 0.7


Suggested change

, unix-compat >= 0.6 && < 0.7

, unix-compat >= 0.5.4 && < 0.6

Yup, that should do it

That'd did it

src/PostgREST/AppState.hs

also store them in AppState simplify Admin.reachMainApp

steve-chavez · 2023-11-23T22:50:43Z

src/PostgREST/Admin.hs

 -- The code for resolving the "*4", "!4", "*6", "!6", "*" special values is taken from
 -- https://hackage.haskell.org/package/streaming-commons-0.2.2.4/docs/src/Data.Streaming.Network.html#bindPortGenEx


This is no longer the case right? Will remove.

steve-chavez reviewed Nov 7, 2023

View reviewed changes

src/PostgREST/Unix.hs Show resolved Hide resolved

develop7 mentioned this pull request Nov 10, 2023

server-port can be 0 PostgREST/postgrest-docs#695

Merged

develop7 marked this pull request as ready for review November 17, 2023 07:31

develop7 requested review from steve-chavez, wolfgangwalther and taimoorzaeem November 17, 2023 11:51

steve-chavez reviewed Nov 17, 2023

View reviewed changes

steve-chavez reviewed Nov 22, 2023

View reviewed changes

postgrest.cabal Outdated Show resolved Hide resolved

develop7 requested a review from steve-chavez November 22, 2023 16:38

steve-chavez reviewed Nov 22, 2023

View reviewed changes

postgrest.cabal Outdated Show resolved Hide resolved

steve-chavez reviewed Nov 22, 2023

View reviewed changes

src/PostgREST/AppState.hs Show resolved Hide resolved

develop7 added 8 commits November 23, 2023 11:14

app: quick'n'dirty displaying of bound port

3eb028f

postgrest style

eff8e1a

create app sockets ourselves

f7c5697

also store them in AppState simplify Admin.reachMainApp

fix leftover errors

41df536

cabal: add streaming-commons

ab5bc73

spec: fix tests

b47a358

admin: fixed crash in reachMainApp

4e38128

postgrest-style

6081cd6

develop7 and others added 18 commits November 23, 2023 11:14

postgrest-style

394b894

get unix socket runtime check back

17306e3

postgrest-style

44075d4

bind then listen

364f2d2

use setPerms from unix-compat

759094a

style

95f665f

pin unix-compat for warp

50925df

put Unix specifics to Postgrest.Unix

7d21a49

forgot .cabal

846bf5d

fix windows build

30495e6

fix installSignalHandlers stub

93c3953

move domain socket avail check to Unix

b09804e

exe: drop network from deps

b9642a3

style

7fe26dc

unpin unix-compat

e56e857

Update postgrest.cabal

56d524c

Update postgrest.cabal

9df15e1

test/io: test random port assignment

77a9345

develop7 force-pushed the fix_print-actual-port branch from a57e6bb to 77a9345 Compare November 23, 2023 10:15

style

904f01d

steve-chavez reviewed Nov 23, 2023

View reviewed changes

delete outdated comment add CHANGELOG

bee8c8b

steve-chavez merged commit df97a50 into PostgREST:main Nov 23, 2023
31 checks passed

steve-chavez mentioned this pull request Dec 19, 2023

Admin server returns HTTP 503 in 12.x #3128

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Display an actual TCP port app is bound to #3034

Display an actual TCP port app is bound to #3034

develop7 commented Nov 1, 2023 •

edited

Loading

steve-chavez commented Nov 1, 2023

develop7 commented Nov 2, 2023

develop7 commented Nov 2, 2023

develop7 commented Nov 2, 2023

develop7 commented Nov 3, 2023

steve-chavez commented Nov 5, 2023

develop7 commented Nov 6, 2023

develop7 commented Nov 9, 2023

develop7 commented Nov 9, 2023

develop7 commented Nov 9, 2023

steve-chavez Nov 17, 2023 •

edited

Loading

develop7 Nov 17, 2023 •

edited

Loading

develop7 Nov 22, 2023

steve-chavez Nov 22, 2023

develop7 Nov 22, 2023

develop7 Nov 22, 2023

steve-chavez Nov 23, 2023

	, unix-compat >= 0.6 && < 0.7
	, unix-compat >= 0.5.4 && < 0.6

		-- The code for resolving the "4", "!4", "6", "!6", "*" special values is taken from
		-- https://hackage.haskell.org/package/streaming-commons-0.2.2.4/docs/src/Data.Streaming.Network.html#bindPortGenEx

Display an actual TCP port app is bound to #3034

Display an actual TCP port app is bound to #3034

Conversation

develop7 commented Nov 1, 2023 • edited Loading

steve-chavez commented Nov 1, 2023

develop7 commented Nov 2, 2023

develop7 commented Nov 2, 2023

develop7 commented Nov 2, 2023

develop7 commented Nov 3, 2023

steve-chavez commented Nov 5, 2023

develop7 commented Nov 6, 2023

develop7 commented Nov 9, 2023

develop7 commented Nov 9, 2023

develop7 commented Nov 9, 2023

steve-chavez Nov 17, 2023 • edited Loading

Choose a reason for hiding this comment

develop7 Nov 17, 2023 • edited Loading

Choose a reason for hiding this comment

develop7 Nov 22, 2023

Choose a reason for hiding this comment

steve-chavez Nov 22, 2023

Choose a reason for hiding this comment

develop7 Nov 22, 2023

Choose a reason for hiding this comment

develop7 Nov 22, 2023

Choose a reason for hiding this comment

steve-chavez Nov 23, 2023

Choose a reason for hiding this comment

develop7 commented Nov 1, 2023 •

edited

Loading

steve-chavez Nov 17, 2023 •

edited

Loading

develop7 Nov 17, 2023 •

edited

Loading