Skip to content

Commit 4de7709

Browse files
Jules Ivanicguizmaii
Jules Ivanic
authored and
guizmaii
committed
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 4de7709

6 files changed

Lines changed: 69 additions & 58 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: 3 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -61,7 +61,7 @@ final private class FeatureFlagRegistryLive(
6161
override def getClient(domain: String): UIO[FeatureFlags] =
6262
lock.withPermit {
6363
clients.get.map(_.get(domain)).flatMap {
64-
case Some(c) => ZIO.succeed(c)
64+
case Some(c) => Exit.succeed(c)
6565
case None => createDomainClient(domain)
6666
}
6767
}
@@ -74,10 +74,7 @@ final private class FeatureFlagRegistryLive(
7474
case Some(client) =>
7575
client
7676
.setProvider(provider)
77-
.tapBoth(
78-
_ => ZIO.unit,
79-
_ => providers.update(_ + (domain -> provider))
80-
)
77+
.tap(_ => providers.update(_ + (domain -> provider)))
8178
case None =>
8279
providers.update(_ + (domain -> provider))
8380
}
@@ -87,7 +84,7 @@ final private class FeatureFlagRegistryLive(
8784
override def defaultClient: UIO[FeatureFlags] =
8885
lock.withPermit {
8986
defaultRef.get.flatMap {
90-
case Some(c) => ZIO.succeed(c)
87+
case Some(c) => Exit.succeed(c)
9188
case None => createDefaultClient
9289
}
9390
}

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

Lines changed: 30 additions & 25 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")
@@ -776,7 +783,7 @@ final private[openfeature] class FeatureFlagsLive(
776783
Some((newCtx, hints))
777784
} else None
778785
}
779-
.catchAll(_ => ZIO.none)
786+
.catchAll(_ => Exit.none)
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: 25 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -267,17 +267,16 @@ object FeatureHook {
267267
// so that `before` can return None and avoid corrupting the compose pipeline's
268268
// context-modified tracking.
269269
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 {
270+
ZIO.suspendSucceed {
271+
ctx.hookData.set(startTimeKey, java.lang.System.nanoTime())
272+
beforeLevel match {
274273
case Some(level) =>
275274
annotate(baseAnnotations(ctx) ++ contextAnnotations(ctx))(
276275
logAtLevel(level, s"Evaluating flag '${ctx.flagKey}'")
277-
)
278-
case None => ZIO.unit
276+
).as(None)
277+
case None => Exit.none
279278
}
280-
} yield None
279+
}
281280

282281
private def elapsed(ctx: HookContext): Duration = {
283282
val end = java.lang.System.nanoTime()
@@ -361,24 +360,26 @@ object FeatureHook {
361360
private val startTimeKey = TypedKey[Long]("metricsDetailed.startTime")
362361

363362
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-
}
363+
ZIO.suspendSucceed {
364+
ctx.hookData.set(startTimeKey, java.lang.System.nanoTime())
365+
Exit.none
366+
}
375367

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-
}
368+
override def after[A](ctx: HookContext, details: FlagResolution[A], hints: HookHints): UIO[Unit] =
369+
ZIO.suspendSucceed {
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+
}
375+
376+
override def error(ctx: HookContext, err: FeatureFlagError, hints: HookHints): UIO[Unit] =
377+
ZIO.suspendSucceed {
378+
val end = java.lang.System.nanoTime()
379+
val start = ctx.hookData.get(startTimeKey).getOrElse(end)
380+
val duration = Duration.fromNanos(end - start)
381+
onError(ctx, err, duration)
382+
}
382383
}
383384

384385
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)

0 commit comments

Comments
 (0)