Add option to allow specifying address to bind to #51
Conversation
|
Well, you're creating |
rbjorklin
force-pushed
the
allow-specifying-bind-addr
branch
from
305bdbb to
0cf500c
Compare
|
Beej's Guide to the rescue. The PR works now 😃 |
rbjorklin
force-pushed
the
allow-specifying-bind-addr
branch
3 times, most recently
from
d4745f9 to
1ad0a47
Compare
talex5
left a comment
talex5
left a comment
There was a problem hiding this comment.
It might be better to have a single option e.g. --listen-prometheus=127.0.0.1:9090. At the moment, it's unclear what should happen if you give e.g. an address and no port. Also, since we have defaults for both host and port, we could also allow just --listen-prometheus to turn it on without specifying either value.
Will need some compatibility handling for the old port option (and an error if you give both).
app/prometheus_unix.ml
Outdated
| let () = Lwt_unix.setsockopt socket SO_REUSEADDR true in | ||
| let mode = `TCP (`Socket socket) in | ||
| let callback = Server.callback in | ||
| let () = Lwt.async (fun () -> Lwt_unix.bind socket addrinfo.ai_addr) in |
There was a problem hiding this comment.
This isn't going to be reliable. It should be done inside thread, not racing with it.
There was a problem hiding this comment.
Made the port binding synchronous, this should be a lot more reliable.
rbjorklin
force-pushed
the
allow-specifying-bind-addr
branch
from
ad4b57a to
9a048dc
Compare
rbjorklin
force-pushed
the
allow-specifying-bind-addr
branch
2 times, most recently
from
bdfdf4f to
f8a722a
Compare
rbjorklin
force-pushed
the
allow-specifying-bind-addr
branch
from
f8a722a to
abbb7fe
Compare
2 participants
I have attempted to maintain full backwards compatibility while also allowing one to specify the bind address with
--listen-prometheus-addrwhich otherwise just defaults to0.0.0.0.Unfortunately something in conduit_lwt_server throws
Uncaught exception accepting connection: Unix.Unix_error(Unix.EINVAL, "accept", "")when trying to use this no matter what address is being used.I'm hoping the maintainers of this repo can shed some light on what might be going on otherwise I will open an issue against conduit as I think the implementation in this PR should work.
EDIT: I just came across this bit regarding the use of
cloexec, thoughts?