Repository navigation
TracingPlatform performs undefined behavior in JS that makes it unusable in Wasm #4644
Description
Activity
This is probably the same issue as #4608. If I understand it correctly, this code in
TracingPlatformis also incorrect in regular-JS, and not just WASM. (It just "happens to work" on regular-JS, and crashes on WASM.) That is good to know. Maybe @armanbilge remembers why was it written this way originally. /cc @samspillsIf I understand it correctly, this code in
TracingPlatformis also incorrect in regular-JS, and not just WASM. (It just "happens to work" on regular-JS, and crashes on WASM.)Yes, you understand correctly :)
Reacted by Daniel UrbanMaybe @armanbilge remembers why was it written this way originally.
I was aware that this breaks abstractions and relies on implementation details to work. This was the only way I could figure out how to support cached tracing on Scala.js on JavaScript when I worked on it 5 years ago in #2207. (Apologies for not documenting as a code comment.)
TracingPlatformis also incorrect in regular-JSI'm not sure what you mean by this.
If there is a way to support cached tracing on Scala.js-on-JS without relying on undefined behavior and implementation details, we should definitely do that. In any case, we should definitely implement this in a way that does not prevent the use of the Wasm platform.
TracingPlatformis also incorrect in regular-JSI'm not sure what you mean by this.
That it only "happens to work" given the current implementation of Scala.js. Any upgrade of Scala.js could break that abstraction breach without even a release note item.
If there is a way to support cached tracing on Scala.js-on-JS without relying on undefined behavior and implementation details, we should definitely do that. In any case, we should definitely implement this in a way that does not prevent the use of the Wasm platform.
Could you describe, or point me to, what that feature is trying to achieve in the first place?
@sjrd Looking at the JVM implementation of the same trait is probably the clearest way to piece together the intent. The full guts of what's going on here…
When users call methods like
maporflatMapor similar, we want to capture the call site in order to build a trace frame. This in turn is what allows us to implement fiber tracing, which underlies Cats Effect's enhanced exceptions and fiber dumps. Without this functionality, every exception occurring within Cats Effect would have almost exactly the same useless stack trace due to the trampoline. There are, to the best of my knowledge, three ways of implementing this type of functionality:- Implicit evidence. Used by Kyo, ZIO 2, Munit, and others
- Bytecode introspection. Used by Akka and ZIO 1
- Exception stack trace walking. Used by Specs 1 (and 2 in many cases), Scala Test (in most cases), JUnit (and most other Java test frameworks), and Cats Effect 2 and 3
All of these have some serious tradeoffs. The implicit evidence is the most pleasing from a performance standpoint, but it pollutes type signatures and defies abstraction (e.g.
Monad#flatMaphas no such evidence). Bytecode introspection gives you the most flexibility on semantics, but it's very expensive, doesn't always yield the answer the user expects, and can run you into some interesting issues when users have custom classloaders or agents. Stack walking is nice because it gives you the precise runtime call path (similar to implicit evidence), and in fact can give you the whole call path allowing for a sort of two-dimensional stack trace, but it's extremely expensive and requires some careful allowlisting to avoid unintuitive artifacts.The main tradeoff of the latter two options is runtime cost, since in theory this computation needs to be performed on every call to
flatMap(and similar). ZIO 1, CE2, and CE3 all take advantage of the same trick in order to mitigate this problem: caching.The trick is that
flatMap(and similar) are usually invoked with call-site-specific closures, meaning that we can inspect the runtimeClassof theFunction1to derive a cache key which is unique to the static call site. With a little bit of shenaniganry, we can even do the same thing for by-name functions, allowing us to extend tracing coverage a bit. SincegetClassis extremely cheap on the JVM, and the number of call sites in an application is generally very bounded, the cache is surprisingly small and the hit rate almost immediately reaches 100% once the application is warmed.The problem is that function values do not have classes on Scala.js. :D The code you're looking at is designed to work around this by fishing out a different property which happens to be call-site specific and efficient to access.
So in other words, all of this is kind of straddling the line on behavior. On the one hand, it's really important functionality. Enhanced exceptions are genuinely one of our most important practical production features. On the other hand, even on the JVM this is technically relying on implementation details of the Scala bytecode representation. We're relatively comfortable relying on such implementation details if needed in this layer (which is why we engaged in this pretty obviously dodgy hackery in the first place), but obviously it has limits.
Very open to suggestions on better ways to achieve this same functionality.
I don't think there is a user-space solution.
IMO, your best course of action would be a link-time IR-rewriting plugin. Wrap the
ClearableLinkerof Scala.js with a custom one. See
https://github.com/scala-js/scala-js/tree/main/sbt-plugin/src/sbt-test/linker/custom-linker
for an example.In the plugin, override
def linkto patch the IR before shipping it to the standardLinker. Find all theNewLambdanodes that targetAbstractFunction0orAbstractFunction1(potentially restricted to call sites of interest). Rewrite them to aNewof a custom class that you add to your runtime. That class can hold a field with a call-site identity of your choosing. Since you now control theNewnode, you can inject the identity.In your
TracingPlatformruntime, type-test for your custom class. If it's an instance, fetch the identity field; otherwise usegetClass()like on the JVM.It is some amount of work, but it would be completely reliable.
That's not something users can incorporate as a library dependency though, which is a really significant tradeoff and well outside the normal Cats Effect adoption model. NGL, the center of mass in my thinking here still biases towards piling more hacks on top of the hacks and simply detecting WASM (and potential future releases which alter this encoding). The identity will always be there somewhere because of what closures fundamentally are. I'd certainly prefer something in userspace (or userspace-ish, like the JVM solution), but either way I don't think we can force users to import build plugins.
On Wasm the lambda really does not have an identity.
funcrefis not a subtype ofeqref, so you can't perform an identity test on a lambda.Even on JS you are using
toString(), which is not an identity. You could get the same "identity" for two different call sites if they happen to render to the same JS source code.I'm trying to get to a successful workaround for this issue. What I have so far is at #4691. My goal is to have non-wasm scala-js "continue to happen to work", and wasm to degrade gracefully by "cached" working like "off". (This is obviously not ideal, but maybe better than the current situation, where it crashes.)
As far as I can tell, #4691 doesn't invoke any undefined behavior. Tracing may or may not work, but degrades gracefully. The tests passing (and my manual testing) suggests it continues to happen to work on non-wasm scala-js. Manual testing shows that the crash in #4608 doesn't happen any more on wasm.
(Making it actually work on wasm, or at least adding wasm to CI is left for future work.)
cats-effect/core/js/src/main/scala/cats/effect/tracing/TracingPlatform.scala
Lines 28 to 31 in 18fdf52
These lines attempt to break through Scala.js abstractions. They access JS properties of a Scala.js object. That is undefined behavior.
When linking for WebAssembly, they actually crash, since Scala.js objects really don't have JS properties.
Since
Tracingis always called byIO.async, it makes the latter completely unusable with Scala.js-on-Wasm.There does not appear to be any workaround.