Skip to content
This repository has been archived by the owner on Sep 14, 2023. It is now read-only.

combining Runes produces inconsistent behavior #1136

Closed
josepot opened this issue Jul 5, 2023 · 3 comments
Closed

combining Runes produces inconsistent behavior #1136

josepot opened this issue Jul 5, 2023 · 3 comments
Labels
bug Something is broken

Comments

@josepot
Copy link
Contributor

josepot commented Jul 5, 2023

Bug Report

This document claims that Runes maintain consistency by retaining information about the sources of updates. However, despite reading the document multiple times, I still struggle to understand what exactly this "consistent behavior" entails. In fact, the more I delve into it, the more inconsistencies I discover when combining different Runes through tuple.

To bring clarity and benefit the entire community, I would greatly appreciate it if the CAPI team could provide a precise and technical explanation of what they mean by "consistent behavior." It is essential to understand the specific details of this behavior.

During my initial reading of the document, I formed an impression that the so-called "consistent behavior" could be described as follows:

When different Runes are combined through tuple and emit values during the same task-queue iteration, the resulting Rune produced by tuple will wait until the end of that task-queue iteration. It will then emit a tuple containing the latest value from each input Rune, consolidating the values produced thus far.

This behavior appears to have practical utility for many use cases. While I don't necessarily consider it a good default behavior, it does make some sense if this is the intended behavior of tuple.

However, upon closer examination, I find that the behavior exhibited in the first example aligns with the behavior I described. Yet, the other two examples deviate from this behavior. This inconsistency adds to the confusion and further complicates understanding.

In conclusion, I strongly believe that a precise and technical explanation of the "consistent behavior" exhibited by Runes, particularly in relation to combining Runes through tuple, would greatly benefit all users of CAPI.

Let's examine the examples provided. The first example aligns with the definition I previously proposed, which is promising:

import { Rune } from "capi"
import { delay } from "https://deno.land/[email protected]/async/mod.ts"

// Some kind of event source; this might correspond to, say, new blocks from a given chain
const a = timer(1000).map((n) => `a${n}`) // a0, a1, a2, ...

// Two different derived asynchronous queries based off of `a`
const ax = a.map((value) => delay(500).then(() => `${value}.x`)) // a0.x, a1.x, a2.x, ...
const ay = a.map((value) => delay(500).then(() => `${value}.y`)) // a0.y, a1.y, a2.y, ...

// Combine the results from each to process in some way
const az = Rune.tuple([ax, ay])

const start = Date.now()
for await (const value of az.iter()) {
  console.log(Date.now() - start, value)
}

// Every update is based on only one value from `a`; this is good.
/*
  503 [ "a0.x", "a0.y" ]
  1505 [ "a1.x", "a1.y" ]
  2507 [ "a2.x", "a2.y" ]
  3509 [ "a3.x", "a3.y" ]
  4512 [ "a4.x", "a4.y" ]
  ...
*/

However, the second example raises concerns. It appears to either be non-compliant with the defined behavior, or there may be a significant issue with the Rune's scheduler or the implementation of the timer function (although the latter seems more likely):

import { Rune } from "capi"
import { delay } from "https://deno.land/[email protected]/async/mod.ts"

const a = timer(1000).map((n) => `a${n}`) // a0, a1, a2, ...
const b = timer(1500).map((n) => `b${n}`) // b0, b1, b2, ...

// Combine them with `ls` like before:
const ab = Rune.tuple([a, b])

const start = Date.now()
for await (const value of ab.iter()) {
  console.log(Date.now() - start, value)
}

// This still has the correct behavior, without having to change the combinator:
/*
  2 [ "a0", "b0" ]
  1003 [ "a1", "b0" ]
  1506 [ "a1", "b1" ]
  2005 [ "a2", "b1" ]
  3007 [ "a3", "b1" ] // Why was this tuple emitted? It seems inconsistent with the behavior of the first example
  3007 [ "a3", "b2" ] // because this one should have been queued in the same macrotask :/
  4009 [ "a4", "b2" ]
  4509 [ "a4", "b3" ]
  5010 [ "a5", "b3" ]
  6011 [ "a5", "b4" ] // This value should have not been emitted according to the initial claim
  6012 [ "a6", "b4" ] // Only this one should have been emitted.
  7013 [ "a7", "b4" ]
  7514 [ "a7", "b5" ]
  8021 [ "a8", "b5" ]
  9020 [ "a8", "b6" ] // likewise
  9023 [ "a9", "b6" ] // Only this one should have been emitted.
  // ^ A 3 ms discrepancy after so few emitted values seems to indicate that there is some serious overhead going
  // on with the Runes, which will IMO not help at producing consistent behaviors.   
*/

As for the third example, it presents its own set of puzzling aspects:

import { Rune } from "capi"
import { delay } from "https://deno.land/[email protected]/async/mod.ts"

const a = timer(1000).map((n) => `a${n}`) // a0, a1, a2, ...
const b = timer(1500).map((n) => `b${n}`) // b0, b1, b2, ...
const c = timer(2500).map((n) => `c${n}`) // c0, c1, c2, ...

const ab = Rune.tuple([a, b])
const bc = Rune.tuple([b, c])

// The combinator is the same as always
const allTogetherNow = Rune.tuple([ab, bc])

const start = Date.now()
for await (const value of allTogetherNow.iter()) {
  console.log(Date.now() - start, value)
}

// And everything works correctly; `a` is faster than `b` is faster than `c`,
// and `b`'s value is always consistent:
/*
  2 [ [ "a0", "b0" ], [ "b0", "c0" ] ]
  1004 [ [ "a1", "b0" ], [ "b0", "c0" ] ]
  1504 [ [ "a1", "b1" ], [ "b1", "c0" ] ]
  2005 [ [ "a2", "b1" ], [ "b1", "c0" ] ]
  2502 [ [ "a2", "b1" ], [ "b1", "c1" ] ]
  3005 [ [ "a2", "b2" ], [ "b2", "c1" ] ] // Why is this one here?
  3007 [ [ "a3", "b2" ], [ "b2", "c1" ] ] // Only this one should have appeared... Also why did this one come so late? What's going on?
  4008 [ [ "a4", "b2" ], [ "b2", "c1" ] ]
  4508 [ [ "a4", "b3" ], [ "b3", "c1" ] ]
  5005 [ [ "a4", "b3" ], [ "b3", "c2" ] ] // WHOOPS! 
  5009 [ [ "a5", "b3" ], [ "b3", "c2" ] ] 
  6009 [ [ "a5", "b4" ], [ "b4", "c2" ] ] // Another emitted tuple that shouldn't be here
  6011 [ [ "a6", "b4" ], [ "b4", "c2" ] ]
  7013 [ [ "a7", "b4" ], [ "b4", "c2" ] ]
  7508 [ [ "a7", "b4" ], [ "b4", "c3" ] ] // another one
  7510 [ [ "a7", "b5" ], [ "b5", "c3" ] ]
  8013 [ [ "a8", "b5" ], [ "b5", "c3" ] ]
  9013 [ [ "a8", "b6" ], [ "b6", "c3" ] ] // and another one
  9014 [ [ "a9", "b6" ], [ "b6", "c3" ] ]
*/

For what it's worth, I'm pretty sure that the "consistent behavior" defined earlier accurately captures the intention of the tuple Rune combinator. However, the lack of a proper scheduler in Runes, combined with a potentially naive implementation of the timer function, may be the root cause of these inconsistencies.

Interestingly, achieving this "consistent behavior" with RxJS is remarkably straightforward. I will delve into the details of this comparison in a separate discussion to provide a comprehensive explanation.

@josepot josepot added the bug Something is broken label Jul 5, 2023
@robocapi robocapi added this to Capi Jul 5, 2023
@tjjfvi
Copy link
Contributor

tjjfvi commented Jul 7, 2023

I'm merging all of the conversation into this discussion.

@tjjfvi tjjfvi closed this as not planned Won't fix, can't repro, duplicate, stale Jul 7, 2023
@github-project-automation github-project-automation bot moved this to Done in Capi Jul 7, 2023
@paritytech paritytech locked and limited conversation to collaborators Jul 7, 2023
@josepot
Copy link
Contributor Author

josepot commented Jul 7, 2023

I'm merging all of the conversation into this discussion.

I believe it is crucial to distinguish the broader discussion about Runes from the specific bug mentioned here.

Based on the documentation, it appears that the observed behavior where Runes that were expected to update together are not actually updating together should be considered a bug. I think that this discrepancy between the promised behavior and the actual behavior highlights an issue that needs to be addressed and resolved.

@tjjfvi
Copy link
Contributor

tjjfvi commented Jul 7, 2023

As I cover in #1145 (comment), your description of Rune's intended behavior is not accurate. Rune is working as intended.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Something is broken
Projects
Status: Done
Development

No branches or pull requests

2 participants