Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add fs2.StreamApp from http4s #982

Merged
merged 3 commits into from
Nov 21, 2017
Merged

Conversation

aeons
Copy link
Member

@aeons aeons commented Nov 21, 2017

This is a first attempt at a StreamApp in fs2. I've taken the one in http4s, tried to conform to the code style in fs2 and named it fs2.StreamApp. I've also added fs2.ExitCode. It could live in the same file as StreamApp, to keep the entire machinery contained in one file.

I have added the tests we had in http4s as well.

Closes #973

@pchlupacek
Copy link
Contributor

@aeons cool and thanks for that. Just some general comments:

I would consider to have StreamApp around the type F instead any IO related code just there, if possible.

Do you think it may be defined without the need of ExecutionContext at all?

Instead of ExitCode algebra, do you think we can provide just Int to be consistent with System.exit ?

I am not completely sure about the requestShutdownHook mechanics. My understanding is that it may be used to give time for the stream to be gracefully shutdown on kill <signal>. I am not sure if users will know how to use it within the def stream definition.

It would be excellent if we would be able do develop functionality that will interpret these signals, and provide something like Stream[F, SYS_SIGNAL] to be consulted within the def stream.

@aeons
Copy link
Member Author

aeons commented Nov 21, 2017

The IO stuff is only for the internal signals, without those I couldn't find a way to actually run the resulting program to get an exit code out. If you have a solution I'm all ears, I've spent way too long on it :)

The ExecutionContext is needed, as I understand it, but I think @rossabaker has a better grasp on that part.

WRT. ExitCode, I don't have any strong preferences. Since it's turned into a byte at the OS-level, having a type with constants for error and success made sense in http4s. sys.exit takes an int, so we are constraining it more than that.

The requestShutdown effect will interrupt the stream and allow it to do any clean up it needs before exiting the JVM. It can be ignored if you don't need that use case.
The JVM shutdownHook is what allows this to happen.
It's not explicitly for handling kill signals.

Most of the discussions on StreamApp in http4s have been on Gitter unfortunately, but there are some issues that might be interesting:
http4s/http4s#1417
http4s/http4s#1444
http4s/http4s#1467

cc @rossabaker @ChristopherDavenport

@mpilquist
Copy link
Member

Looks good to me. I'm fine with keeping ExitCode but I'd like to move it in to the StreamApp companion to keep the top-level fs2 package cleaner (or alternatively, I suppose we could have an fs2.app package with ExitCode and StreamApp, though I lean the first way).

@aeons
Copy link
Member Author

aeons commented Nov 21, 2017

Sure, that's where it lived to begin with in http4s, but since it was referenced from the server builders, it was agreed to move it out.

Since it will only be referred to from StreamApp in fs2, having it live there makes sense.

@mpilquist
Copy link
Member

Oh, super minor thing but can we do TitleCase for the constants Success and Error?

@mpilquist
Copy link
Member

LGTM. I'll give @pchlupacek some time to review your response from earlier but otherwise I think this is ready for merge.

@pchlupacek
Copy link
Contributor

ok no problem with merging this I perhaps would like to suggest some improvements on handling unix signal and shutdownhook later, but lets start with this.

@mpilquist mpilquist merged commit cbc9ac6 into typelevel:series/0.10 Nov 21, 2017
@aeons aeons deleted the topic/streamapp branch December 6, 2017 08:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants