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

Call Driver#.start only when Server#.install is first called (#2381) #2872

Merged
merged 3 commits into from
Jun 23, 2024

Conversation

masonedmison
Copy link
Contributor

@masonedmison masonedmison commented May 28, 2024

/claim #2381


override def port: Int = bindPort
override def port[R]: URIO[R, Int] = serverStarted.await.orDie
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

serverStarted will block indefinitely until install is called for the first time. Should we add a comment that mentions this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, I don't love that this now returns a ZIO -- I couldn't think of any way around this since port may or may not be present depending on if the driver has been started.

ZIO.environment[R].flatMap(env => driver.addApp(httpApp, env.prune[R]))
for {
_ <- initialInstall.succeed(())
_ <- serverStarted.await.orDie
Copy link
Contributor Author

@masonedmison masonedmison May 28, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My opinion here was that if we fail to start the driver for any reason that this error should be raised as a "defect" -- this seems like an unrecoverable error to me but please let me know if you disagree.

Copy link

algora-pbc bot commented May 28, 2024

💵 To receive payouts, sign up on Algora, link your Github account and connect with Stripe/Alipay.

@codecov-commenter
Copy link

Codecov Report

Attention: Patch coverage is 75.00000% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 64.47%. Comparing base (3310828) to head (85395f9).
Report is 15 commits behind head on main.

Files Patch % Lines
...o-http/shared/src/main/scala/zio/http/Server.scala 75.00% 1 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2872      +/-   ##
==========================================
- Coverage   64.64%   64.47%   -0.17%     
==========================================
  Files         156      156              
  Lines        9113     9143      +30     
  Branches     1584     1605      +21     
==========================================
+ Hits         5891     5895       +4     
- Misses       3222     3248      +26     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@jdegoes
Copy link
Member

jdegoes commented Jun 4, 2024

You got one failure:

warning: integrate-with-zio-config.md:299:69: dead code following this construct
  val customized: ZLayer[Config & NettyConfig, Throwable, Server] = ???
                                                                    ^^^
error: testing-http-apps.md:110:36: type mismatch;
 found   : zio.URIO[Any,Int]
    (which expands to)  zio.ZIO[Any,Nothing,Int]
 required: Int
          .get(url = URL.root.port(port))
                                   ^^^^
error: testing-http-apps.md:122:48: value url is not a member of Any
        helloResponse    <- client(Request.get(testRequest.url / "hello" / "world"))
                                               ^^^^^^^^^^^^^^^
warning: reference/overview.md:122:85: Unknown link 'reference/reference/handler.md', did you mean 'reference/handler.md'?
There are several ways to create a `Handler`, to learn more about handlers, see the [Handlers](reference/handler.md) page.
                                                                                    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
info: Compiled in 11.21s (2 errors, 12 warnings)
[error] java.lang.RuntimeException: mdoc failed
[error] 	at scala.sys.package$.error(package.scala:27)
[error] 	at mdoc.SbtMain$.main(Main.scala:30)
[error] 	at mdoc.SbtMain.main(Main.scala)
[error] 	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
[error] 	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:77)
[error] 	at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
[error] 	at java.base/java.lang.reflect.Method.invoke(Method.java:568)
[error] stack trace is suppressed; run last docs / Compile / runMain for the full output
[error] (docs / Compile / runMain) mdoc failed
[error] Total time: 53 s, completed May 28, [202](https://github.com/zio/zio-http/actions/runs/9275509783/job/25520374210?pr=2872#step:5:203)4, 8:09:31 PM

@masonedmison masonedmison force-pushed the defer-start-until-first-install branch from 85395f9 to c2fbe71 Compare June 5, 2024 01:51
@masonedmison
Copy link
Contributor Author

You got one failure:

warning: integrate-with-zio-config.md:299:69: dead code following this construct
  val customized: ZLayer[Config & NettyConfig, Throwable, Server] = ???
                                                                    ^^^
error: testing-http-apps.md:110:36: type mismatch;
 found   : zio.URIO[Any,Int]
    (which expands to)  zio.ZIO[Any,Nothing,Int]
 required: Int
          .get(url = URL.root.port(port))
                                   ^^^^
error: testing-http-apps.md:122:48: value url is not a member of Any
        helloResponse    <- client(Request.get(testRequest.url / "hello" / "world"))
                                               ^^^^^^^^^^^^^^^
warning: reference/overview.md:122:85: Unknown link 'reference/reference/handler.md', did you mean 'reference/handler.md'?
There are several ways to create a `Handler`, to learn more about handlers, see the [Handlers](reference/handler.md) page.
                                                                                    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
info: Compiled in 11.21s (2 errors, 12 warnings)
[error] java.lang.RuntimeException: mdoc failed
[error] 	at scala.sys.package$.error(package.scala:27)
[error] 	at mdoc.SbtMain$.main(Main.scala:30)
[error] 	at mdoc.SbtMain.main(Main.scala)
[error] 	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
[error] 	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:77)
[error] 	at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
[error] 	at java.base/java.lang.reflect.Method.invoke(Method.java:568)
[error] stack trace is suppressed; run last docs / Compile / runMain for the full output
[error] (docs / Compile / runMain) mdoc failed
[error] Total time: 53 s, completed May 28, [202](https://github.com/zio/zio-http/actions/runs/9275509783/job/25520374210?pr=2872#step:5:203)4, 8:09:31 PM

Arg, sorry about that. Fixed and force pushed.

@masonedmison masonedmison force-pushed the defer-start-until-first-install branch from c2fbe71 to f2c35e8 Compare June 5, 2024 01:55
@masonedmison
Copy link
Contributor Author

Hrm, it looks like this step [Build and Test (ubuntu-latest, 2.13.14, temurin@21)](https://github.com/zio/zio-http/actions/runs/9376941323/job/25817708565?pr=2872#logs) is still failing. I will have a look at this tomorrow.

@masonedmison masonedmison requested a review from jdegoes June 8, 2024 21:50
@masonedmison masonedmison force-pushed the defer-start-until-first-install branch from b09f217 to b271586 Compare June 19, 2024 02:07
@jdegoes jdegoes merged commit 9e5cd1e into zio:main Jun 23, 2024
33 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants