Skip to content

Commit bb86ba5

Browse files
Jules Ivanicguizmaii
authored andcommitted
Fix bugs, ZIO anti-patterns, and optimize ZIO runloop performance
- Fix integer overflow in ContextConverter.valueToAttribute for large Long values - Fix FlagValueType.fromFlagType mapping Long and Float to Object instead of Int/Double - Fix Clock.nanoTime / System.nanoTime() inconsistency in hooks - Suspend side effects in ZIO.suspendSucceed instead of eager execution - Use Exit.succeed/Exit.unit/Exit.none inside ZIO combinators for faster runloop - Replace ZIO.collectAllParDiscard with sequential chain for trivial Ref ops - Replace tapBoth with no-op error handler by tap - Use try/catch + NonFatal instead of ZIO.attempt for non-blocking Java calls
1 parent 204f7ad commit bb86ba5

8 files changed

Lines changed: 119 additions & 78 deletions

File tree

core/src/main/scala-2/zio/openfeature/FlagValueType.scala

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,8 @@ object FlagValueType {
2424
case "Boolean" => Boolean
2525
case "String" => String
2626
case "Int" => Int
27+
case "Long" => Int
28+
case "Float" => Double
2729
case "Double" => Double
2830
case _ => Object
2931
}

core/src/main/scala-3/zio/openfeature/FlagValueType.scala

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -17,5 +17,7 @@ object FlagValueType:
1717
case "Boolean" => Boolean
1818
case "String" => String
1919
case "Int" => Int
20+
case "Long" => Int
21+
case "Float" => Double
2022
case "Double" => Double
2123
case _ => Object

core/src/main/scala/zio/openfeature/FeatureFlagRegistry.scala

Lines changed: 12 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -60,34 +60,28 @@ final private class FeatureFlagRegistryLive(
6060

6161
override def getClient(domain: String): UIO[FeatureFlags] =
6262
lock.withPermit {
63-
clients.get.map(_.get(domain)).flatMap {
64-
case Some(c) => ZIO.succeed(c)
63+
clients.get.flatMap(_.get(domain) match {
64+
case Some(c) => Exit.succeed(c)
6565
case None => createDomainClient(domain)
66-
}
66+
})
6767
}
6868

6969
override def setProvider(domain: String, provider: OFFeatureProvider): IO[FeatureFlagError, Unit] =
7070
lock.withPermit {
71-
for {
72-
existing <- clients.get.map(_.get(domain))
73-
_ <- existing match {
74-
case Some(client) =>
75-
client
76-
.setProvider(provider)
77-
.tapBoth(
78-
_ => ZIO.unit,
79-
_ => providers.update(_ + (domain -> provider))
80-
)
81-
case None =>
82-
providers.update(_ + (domain -> provider))
83-
}
84-
} yield ()
71+
clients.get.flatMap(_.get(domain) match {
72+
case Some(client) =>
73+
client
74+
.setProvider(provider)
75+
.tap(_ => providers.update(_ + (domain -> provider)))
76+
case None =>
77+
providers.update(_ + (domain -> provider))
78+
})
8579
}
8680

8781
override def defaultClient: UIO[FeatureFlags] =
8882
lock.withPermit {
8983
defaultRef.get.flatMap {
90-
case Some(c) => ZIO.succeed(c)
84+
case Some(c) => Exit.succeed(c)
9185
case None => createDefaultClient
9286
}
9387
}

core/src/main/scala/zio/openfeature/FeatureFlagsLive.scala

Lines changed: 33 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -5,18 +5,20 @@ import zio.stream._
55
import zio.openfeature.internal.{ClientEvaluator, ContextConverter, ErrorCodeConverter, FeatureFlagsState}
66
import dev.openfeature.sdk.{
77
Client => OFClient,
8-
FeatureProvider => OFFeatureProvider,
9-
FlagEvaluationDetails,
10-
Reason => OFReason,
118
ErrorCode => OFErrorCode,
12-
OpenFeatureAPI,
13-
MutableTrackingEventDetails,
9+
FeatureProvider => OFFeatureProvider,
10+
FlagValueType => JavaFlagValueType,
11+
HookContext => JavaHookContext,
1412
ProviderEvent => JavaProviderEvent,
13+
Reason => OFReason,
1514
EventDetails,
16-
FlagValueType => JavaFlagValueType,
17-
HookContext => JavaHookContext
15+
FlagEvaluationDetails,
16+
MutableTrackingEventDetails,
17+
OpenFeatureAPI
1818
}
19+
1920
import scala.jdk.CollectionConverters._
21+
import scala.util.control.NonFatal
2022

2123
final private[openfeature] class FeatureFlagsLive(
2224
client: OFClient,
@@ -47,7 +49,9 @@ final private[openfeature] class FeatureFlagsLive(
4749
val javaMeta = details.getEventMetadata
4850
if (javaMeta == null || javaMeta.isEmpty) FlagMetadata.empty
4951
else convertImmutableMetadata(javaMeta)
50-
} catch { case _: Exception => FlagMetadata.empty }
52+
} catch {
53+
case _: Exception => FlagMetadata.empty
54+
}
5155

5256
val readyHandler: java.util.function.Consumer[EventDetails] = details =>
5357
Unsafe.unsafe { implicit u =>
@@ -195,7 +199,7 @@ final private[openfeature] class FeatureFlagsLive(
195199
case ProviderStatus.Fatal => ZIO.fail(FeatureFlagError.ProviderFatal)
196200
case ProviderStatus.NotReady => ZIO.fail(FeatureFlagError.ProviderNotReady(ProviderStatus.NotReady))
197201
case ProviderStatus.Error => ZIO.fail(FeatureFlagError.ProviderNotReady(ProviderStatus.Error))
198-
case _ => ZIO.unit
202+
case _ => Exit.unit
199203
}
200204

201205
// Context is already merged by evaluateWithDetails before entering the hook pipeline
@@ -609,7 +613,7 @@ final private[openfeature] class FeatureFlagsLive(
609613
override def currentEvaluatedFlags: UIO[Map[String, zio.openfeature.FlagEvaluation[_]]] =
610614
state.transactionRef.get.flatMap {
611615
case Some(ts) => ts.getEvaluations
612-
case None => ZIO.succeed(Map.empty)
616+
case None => Exit.succeed(Map.empty)
613617
}
614618

615619
override def events: ZStream[Any, Nothing, ProviderEvent] =
@@ -715,13 +719,16 @@ final private[openfeature] class FeatureFlagsLive(
715719

716720
private def getProviderHooks: UIO[List[FeatureHook]] =
717721
providerRef.get.flatMap { p =>
718-
ZIO
719-
.attempt {
720-
val javaHooks = p.getProviderHooks
722+
try {
723+
val javaHooks = p.getProviderHooks
724+
val res =
721725
if (javaHooks == null || javaHooks.isEmpty) Nil
722726
else javaHooks.asScala.toList.map(wrapJavaHook)
723-
}
724-
.catchAll(e => ZIO.logWarning(s"Failed to get provider hooks: ${e.getMessage}").as(Nil))
727+
Exit.succeed(res)
728+
} catch {
729+
case NonFatal(e) =>
730+
ZIO.logWarningCause(s"Failed to get provider hooks", Cause.fail(e)).as(Nil)
731+
}
725732
}
726733

727734
@scala.annotation.nowarn("msg=deprecated")
@@ -766,17 +773,17 @@ final private[openfeature] class FeatureFlagsLive(
766773
val hook = javaHook.asInstanceOf[dev.openfeature.sdk.Hook[Any]]
767774
new FeatureHook {
768775
override def before(ctx: HookContext, hints: HookHints): UIO[Option[(EvaluationContext, HookHints)]] =
769-
ZIO
770-
.attempt {
776+
ZIO.succeed {
777+
try {
771778
val jCtx = toJavaHookContext(ctx)
772779
val jHints = hints.values.map { case (k, v) => k -> v.asInstanceOf[Object] }.asJava
773780
val result = hook.before(jCtx, jHints)
774781
if (result != null && result.isPresent) {
775782
val newCtx = fromJavaEvaluationContext(result.get())
776783
Some((newCtx, hints))
777784
} else None
778-
}
779-
.catchAll(_ => ZIO.none)
785+
} catch { case NonFatal(_) => None }
786+
}
780787

781788
override def after[A](ctx: HookContext, details: FlagResolution[A], hints: HookHints): UIO[Unit] =
782789
ZIO.attempt {
@@ -846,15 +853,13 @@ final private[openfeature] class FeatureFlagsLive(
846853
// Shutdown API (spec 1.6.1, 1.6.2)
847854

848855
override def shutdown: UIO[Unit] =
849-
ZIO.collectAllParDiscard(
850-
List(
851-
state.statusRef.set(ProviderStatus.NotReady),
852-
state.hooksRef.set(List.empty),
853-
state.globalContextRef.set(EvaluationContext.empty),
854-
state.clientContextRef.set(EvaluationContext.empty),
855-
state.trackRecorder.set(List.empty)
856-
)
857-
) *> state.eventHub.shutdown *> ZIO.attemptBlocking(api.shutdown()).ignore
856+
state.statusRef.set(ProviderStatus.NotReady) *>
857+
state.hooksRef.set(List.empty) *>
858+
state.globalContextRef.set(EvaluationContext.empty) *>
859+
state.clientContextRef.set(EvaluationContext.empty) *>
860+
state.trackRecorder.set(List.empty) *>
861+
state.eventHub.shutdown *>
862+
ZIO.attemptBlocking(api.shutdown()).ignore
858863

859864
// Tracking API
860865

core/src/main/scala/zio/openfeature/Hook.scala

Lines changed: 35 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -49,6 +49,9 @@ final class HookData {
4949
def getOrElse[A](key: TypedKey[A], default: => A): A =
5050
get(key).getOrElse(default)
5151

52+
private[openfeature] def getOrNull(key: TypedKey[_]): Any =
53+
data.get().getOrElse(key.name, null)
54+
5255
def remove[A](key: TypedKey[A]): Unit = {
5356
data.updateAndGet(_ - key.name)
5457
()
@@ -267,23 +270,22 @@ object FeatureHook {
267270
// so that `before` can return None and avoid corrupting the compose pipeline's
268271
// context-modified tracking.
269272
override def before(ctx: HookContext, hints: HookHints): UIO[Option[(EvaluationContext, HookHints)]] =
270-
for {
271-
now <- Clock.nanoTime
272-
_ = ctx.hookData.set(startTimeKey, now)
273-
_ <- beforeLevel match {
273+
ZIO.suspendSucceed {
274+
ctx.hookData.set(startTimeKey, java.lang.System.nanoTime())
275+
beforeLevel match {
274276
case Some(level) =>
275277
annotate(baseAnnotations(ctx) ++ contextAnnotations(ctx))(
276278
logAtLevel(level, s"Evaluating flag '${ctx.flagKey}'")
277-
)
278-
case None => ZIO.unit
279+
).as(None)
280+
case None => Exit.none
279281
}
280-
} yield None
282+
}
281283

282-
private def elapsed(ctx: HookContext): Duration = {
283-
val end = java.lang.System.nanoTime()
284-
val start = ctx.hookData.get(startTimeKey).getOrElse(end)
285-
Duration.fromNanos(end - start)
286-
}
284+
private def elapsed(ctx: HookContext): Duration =
285+
ctx.hookData.getOrNull(startTimeKey) match {
286+
case null => Duration.Zero
287+
case start => Duration.fromNanos(java.lang.System.nanoTime() - start.asInstanceOf[Long])
288+
}
287289

288290
override def after[A](ctx: HookContext, details: FlagResolution[A], hints: HookHints): UIO[Unit] =
289291
afterLevel match {
@@ -361,24 +363,28 @@ object FeatureHook {
361363
private val startTimeKey = TypedKey[Long]("metricsDetailed.startTime")
362364

363365
override def before(ctx: HookContext, hints: HookHints): UIO[Option[(EvaluationContext, HookHints)]] =
364-
for {
365-
now <- Clock.nanoTime
366-
_ = ctx.hookData.set(startTimeKey, now)
367-
} yield None
368-
369-
override def after[A](ctx: HookContext, details: FlagResolution[A], hints: HookHints): UIO[Unit] = {
370-
val end = java.lang.System.nanoTime()
371-
val start = ctx.hookData.get(startTimeKey).getOrElse(end)
372-
val duration = Duration.fromNanos(end - start)
373-
onSuccess(ctx, details, duration)
374-
}
366+
ZIO.succeed {
367+
ctx.hookData.set(startTimeKey, java.lang.System.nanoTime())
368+
None
369+
}
375370

376-
override def error(ctx: HookContext, err: FeatureFlagError, hints: HookHints): UIO[Unit] = {
377-
val end = java.lang.System.nanoTime()
378-
val start = ctx.hookData.get(startTimeKey).getOrElse(end)
379-
val duration = Duration.fromNanos(end - start)
380-
onError(ctx, err, duration)
381-
}
371+
override def after[A](ctx: HookContext, details: FlagResolution[A], hints: HookHints): UIO[Unit] =
372+
ZIO.suspendSucceed {
373+
val duration = ctx.hookData.getOrNull(startTimeKey) match {
374+
case null => Duration.Zero
375+
case start => Duration.fromNanos(java.lang.System.nanoTime() - start.asInstanceOf[Long])
376+
}
377+
onSuccess(ctx, details, duration)
378+
}
379+
380+
override def error(ctx: HookContext, err: FeatureFlagError, hints: HookHints): UIO[Unit] =
381+
ZIO.suspendSucceed {
382+
val duration = ctx.hookData.getOrNull(startTimeKey) match {
383+
case null => Duration.Zero
384+
case start => Duration.fromNanos(java.lang.System.nanoTime() - start.asInstanceOf[Long])
385+
}
386+
onError(ctx, err, duration)
387+
}
382388
}
383389

384390
def contextValidator(

core/src/main/scala/zio/openfeature/internal/ContextConverter.scala

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -75,9 +75,13 @@ private[openfeature] object ContextConverter {
7575
if (value.isBoolean) AttributeValue.BoolValue(value.asBoolean())
7676
else if (value.isString) AttributeValue.StringValue(value.asString())
7777
else if (value.isNumber) {
78-
val num = value.asDouble()
79-
if (num == num.toLong.toDouble) AttributeValue.IntValue(num.toInt)
80-
else AttributeValue.DoubleValue(num)
78+
val num = value.asDouble()
79+
val l = num.toLong
80+
val isWholeNumber = num == l.toDouble
81+
if (isWholeNumber) {
82+
if (l >= Int.MinValue && l <= Int.MaxValue) AttributeValue.IntValue(l.toInt)
83+
else AttributeValue.LongValue(l)
84+
} else AttributeValue.DoubleValue(num)
8185
} else if (value.isList) {
8286
val list = value.asList().asScala.map(valueToAttribute).toList
8387
AttributeValue.ListValue(list)

core/src/test/scala/zio/openfeature/EvaluationContextSpec.scala

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -232,6 +232,26 @@ object EvaluationContextSpec extends ZIOSpecDefault {
232232
val ofCtx = zio.openfeature.internal.ContextConverter.toOpenFeature(ctx)
233233
val roundTrip = zio.openfeature.internal.ContextConverter.fromOpenFeature(ofCtx)
234234
assertTrue(roundTrip.targetingKey == None)
235+
},
236+
test("Long value larger than Int.MaxValue survives roundtrip as LongValue") {
237+
val big = Int.MaxValue.toLong + 1L
238+
val ctx = EvaluationContext.empty.withAttribute("big", AttributeValue.LongValue(big))
239+
val ofCtx = zio.openfeature.internal.ContextConverter.toOpenFeature(ctx)
240+
val roundTrip = zio.openfeature.internal.ContextConverter.fromOpenFeature(ofCtx)
241+
assertTrue(roundTrip.get("big") == Some(AttributeValue.LongValue(big)))
242+
},
243+
test("Long value smaller than Int.MinValue survives roundtrip as LongValue") {
244+
val small = Int.MinValue.toLong - 1L
245+
val ctx = EvaluationContext.empty.withAttribute("small", AttributeValue.LongValue(small))
246+
val ofCtx = zio.openfeature.internal.ContextConverter.toOpenFeature(ctx)
247+
val roundTrip = zio.openfeature.internal.ContextConverter.fromOpenFeature(ofCtx)
248+
assertTrue(roundTrip.get("small") == Some(AttributeValue.LongValue(small)))
249+
},
250+
test("Long value within Int range roundtrips as IntValue") {
251+
val ctx = EvaluationContext.empty.withAttribute("n", AttributeValue.LongValue(42L))
252+
val ofCtx = zio.openfeature.internal.ContextConverter.toOpenFeature(ctx)
253+
val roundTrip = zio.openfeature.internal.ContextConverter.fromOpenFeature(ofCtx)
254+
assertTrue(roundTrip.get("n") == Some(AttributeValue.IntValue(42)))
235255
}
236256
)
237257
)

core/src/test/scala/zio/openfeature/HookSpec.scala

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -65,6 +65,14 @@ object HookSpec extends ZIOSpecDefault {
6565
val fvt = FlagValueType.fromFlagType[Double]
6666
assertTrue(fvt == FlagValueType.Double)
6767
},
68+
test("fromFlagType returns Int for Long") {
69+
val fvt = FlagValueType.fromFlagType[Long]
70+
assertTrue(fvt == FlagValueType.Int)
71+
},
72+
test("fromFlagType returns Double for Float") {
73+
val fvt = FlagValueType.fromFlagType[Float]
74+
assertTrue(fvt == FlagValueType.Double)
75+
},
6876
test("fromFlagType returns Object for Map") {
6977
val fvt = FlagValueType.fromFlagType[Map[String, Any]]
7078
assertTrue(fvt == FlagValueType.Object)

0 commit comments

Comments
 (0)