Upgrade to ZIO 2.0.0-RC5 #1194
Upgrade to ZIO 2.0.0-RC5 #1194
Conversation
| def nio: ZLayer[Any, Nothing, ServerChannelFactory] = ZLayer(Live.nio) | ||
| def epoll: ZLayer[Any, Nothing, ServerChannelFactory] = ZLayer(Live.epoll) | ||
| def uring: ZLayer[Any, Nothing, ServerChannelFactory] = ZLayer(Live.uring) | ||
| def auto: ZLayer[Any, Nothing, ServerChannelFactory] = ZLayer(Live.auto) |
There was a problem hiding this comment.
Maybe am I missing something, but shouldn't these be declared as ULayers ? (following the ZIOs declaration logic inside Live)
(same question for ChannelFactory.scala, and the def live: ZLayer[Any, Nothing, DynamicServer] value in DynamicServer.scala)
...
def nio: Ulayer[ServerChannelFactory] = ZLayer(Live.nio)
...There was a problem hiding this comment.
ULayer[A] is a type alias for ZLayer[Any, Nothing, A]. Which one you use is a matter of preference. Some people like the ULayer type alias because it is more terse. Others like to stick with ZLayer because it is fewer names that people need to remember. I tend to prefer not to use the type aliases for layers but here I was just maintaining the existing style.
There was a problem hiding this comment.
Sorry for pestering, any chance we can get this merged in? I'm working on similar PR for softwaremill/tapir and would like to skip the RC6 release of zio-http and go directly to the release that includes this PR
There was a problem hiding this comment.
Well I think we need to figure out what is going on with the test failures here. I'm not quite sure what is going on since all the tests are passing for me locally.
There was a problem hiding this comment.
The tests fail also locally for me.
Something gets slow within the websocket upgrades: (https://github.com/dream11/zio-http/blob/main/zio-http/src/test/scala/zhttp/service/WebSocketServerSpec.scala)
def websocketServerSpec = suite("WebSocketServer") {
suite("connections") {
testM("Multiple websocket upgrades") {
val app = Socket.succeed(WebSocketFrame.text("BAR")).toHttp.deployWS
val codes = ZIO
.foreach(1 to 1024)(_ => app(Socket.empty.toSocketApp).map(_.status)) // here
.map(_.count(_ == Status.SwitchingProtocols))
assertM(codes)(equalTo(1024))
}
}
}Of course, setting the timeout to 30 seconds makes it work,
so does a foreachPar rather than a foreach, but neither of these would be viable solutions (imo).
For now, I still can't get what's going wrong :/
There was a problem hiding this comment.
Hmmm, that's really interesting. Those tests run in a couple seconds for me locally.
There was a problem hiding this comment.
Ok, I guess we might run towards a solution here !
I ran tests, with multiple Scala versions (2.12, 2.13, 3.1), on a few computers, and there's only one where the tests fail:
- Xiaomi Notebook Pro: 🟢 (
CPU: Intel i7 (8 cores) @ 4.900GHz,Memory: 16Go DDR4 3200MT/s) - Thinkpad P15v: 🟢 (
CPU: Intel i7 (16 cores) @ 5.100GHz,Memory: 64Go 2667MT/s) - Macbook Air: 🔴 (
CPU: Intel i7 (2 cores) @ 1.700GHz,Memory: 8Go DDR3 1600MHz)
Updating available memory on the MacBook (via SBT_OPTS), didn't change anything
Hense, the CPU speed, might be responsible for the slow Websockets bulk spawning/updating.
However, it's quite something that this slow down only appears now (since RC5)
There was a problem hiding this comment.
Okay, well increasing the timeout seems to have have at least allowed it to pass CI. Do you have a sense for the relative magnitude of the change? I am surprised by it as well and we can definitely follow up.
Codecov Report
@@ Coverage Diff @@
## zio-series/2.x #1194 +/- ##
===================================================
+ Coverage 39.20% 58.44% +19.23%
===================================================
Files 74 74
Lines 2464 2464
Branches 71 73 +2
===================================================
+ Hits 966 1440 +474
+ Misses 1498 1024 -474
Continue to review full report at Codecov.
|
|
Can we merge this and do another release? |
|
hi. It would be really good if this could be put through to Maven. I've got a ton of RC5-depedent code but can't progress further until this is posted. |
No description provided.