Dream.run: disable ~adjust_terminal behavior

[maxRN]

This adds a special case for handling signals when we are running in a Docker container. If our PID is 1 then we just exit. This solves #174. I'm not entirely sure why the existing approach doesn't work when running in Docker, but I suspect it's got something to do with dream being the PID 1 and then killing itself that Docker doesn't like.

I used this (very bad) Dockerfile to test the implementation:

FROM ocaml/opam@sha256:c900eb55237d14dd57bc10c2f7f9fb09d8b236bcc9572c9f283c9bddfa096fd3 AS build

# Install system dependencies
RUN sudo apk add --update libev-dev openssl-dev gmp-dev pcre-dev libev-dev pkgconf postgresql14-dev sqlite-dev

# Pull the latest OPAM repository updates
RUN cd ~/opam-repository && git pull origin master && opam update
WORKDIR /dream
COPY Makefile .
COPY dream-httpaf.opam .
COPY dream-mirage.opam .
COPY dream-pure.opam .
COPY dream.opam .
COPY dune-project .
RUN opam install dune
RUN make deps
COPY src .
RUN opam exec -- dune build --only-packages dream-pure,dream-httpaf,dream --no-print-directory @install
COPY example .

ENTRYPOINT ["opam", "exec", "--", "dune", "exec", "./1-hello/hello.exe"]

[aantron]

I'm actually thinking of removing that code entirely and having ~adjust_terminal always default to false, and not do any of this. This would help with wide stack traces (when there are long function and file names) and other situations that do occur in development, even my own development, early on, so that I practically always find myself adding ~adjust_terminal:false anyway when coding with Dream. The whole thing with changing terminal settings might have been too much of a gimmick, and if it's causing issues, that seems like an even more urgent argument for getting rid of it.

What do you think?

[aantron]

This also caused a problem that was discussed on Discord recently and I've asked the people there, as well, though it's somewhat dead lately :)

[maxRN]

Truthfully, the truncation of long log lines has also bothered me a bit, and I didn't even know about ~adjust_terminal:false until today. If I had the choice I would at least disable it by default or even get rid of it entirely.

[maxRN]

This also caused a problem that was discussed on Discord recently and I've asked the people there, as well, though it's somewhat dead lately :)

Is the ReasonML channel the "official" channel? I didn't join that Discord and have only been active in the OCaml server, since I don't work with ReasonML. If other people feel the same way it might be a reason why it feels empty.

[aantron]

Would you be willing to remove all this signal code, keep ~adjust_terminal around for backwards compatibility, but ignore it, and see if that also fixes the Docker issue? I'd be happy to merge such a PR.

[aantron]

Is the ReasonML channel the "official" channel? I didn't join that Discord and have only been active in the OCaml server, since I don't work with ReasonML. If other people feel the same way it might be a reason why it feels empty.

It's not exactly official, but it's the most Dream-specific one there is, so I and people have used it.

[maxRN]

I removed all the code related to ~adjust_terminal and set it to be false by default, printing a warning if it's set to true. Not sure about the message, could definitely be improved.

However the Docker bug still occurs without the signal handlers. So I think it must be related to how Lwt does things. I have created a small reproduction of the issue here: https://github.com/maxrn/lwt_test
At least that's the minimum working example I could come up with, I'm not that familiar with Lwt yet ^^

[aantron]

However the Docker bug still occurs without the signal handlers. So I think it must be related to how Lwt does things. I have created a small reproduction of the issue here: https://github.com/maxrn/lwt_test
At least that's the minimum working example I could come up with, I'm not that familiar with Lwt yet ^^

The mystery deepens.

I removed all the code related to ~adjust_terminal and set it to be false by default, printing a warning if it's set to true. Not sure about the message, could definitely be improved.

Many thanks! I am ready to merge this PR as removing ~adjust_terminal, if you don't mind. I'll just push in some tweaks -- no need for anything on your part.

[aantron]

However the Docker bug still occurs without the signal handlers. So I think it must be related to how Lwt does things. I have created a small reproduction of the issue here: https://github.com/maxrn/lwt_test
At least that's the minimum working example I could come up with, I'm not that familiar with Lwt yet ^^

One option would be to report this upstream to the Lwt repo, https://github.com/ocsigen/lwt, especially if you can get rid of the remaining usage of http/af.

[aantron]

If you do, cc me on that issue. I might be able to help spot something during a discussion, as a former Lwt maintainer.

[maxRN]

I created an issue here: ocsigen/lwt#1027 Would be great if you could have a look and possibly add information that I have missed.

[aantron]

Thanks for this!

[aantron]

FYI I ended up with

   if adjust_terminal <> None then begin 
     Error_handler.log.warning (fun log -> 
       log "Dream.run: ~adjust_terminal is deprecated") 
   end; 

see

dream/src/http/http.ml

Lines 685 to 699 in 05a04f5

	?adjust_terminal
	user's_dream_handler =
	let () = if Sys.unix then
	Sys.(set_signal sigpipe Signal_ignore)
	in
	let log = Log.convenience_log in
	(* This should be removed, together with ~adjust_terminal, after a few
	releases. The warning is present since 1.0.0~alpha7. *)
	if adjust_terminal <> None then begin
	Error_handler.log.warning (fun log ->
	log "Dream.run: ~adjust_terminal is deprecated")
	end;

because we want to warn on any use of the option at all, as we'd like to remove it eventually, and any code that uses it will break, no matter which behavior it asks for.

This is just a note, I normally make such small tweaks to avoid round-trips, it's only to spread the ideas around :) Thanks again!

[maxRN]

Ohh, nice! I didn't know that you could do that with with optional parameters. That's really neat.