holdAdd needs to use scheduler.asap, rather than emitting an event directly

[axefrog]

From the architecture wiki:

"A producer source must never produce an event in the same call stack as its run method is called. It must begin producing items asynchronously."

The holdAdd() method is currently violating this policy, resulting in errors in certain edge cases. https://github.com/mostjs/hold/blob/master/src/index.js#L36

To elaborate on the error, in some cases when a single source exists at two places in a composed stream (potentially via most-proxy), it is possible for a sink constructor call to be on the stack, call its upstream source.run(), and then to end up on the stack a second time for an upstream run call. If the upstream run call then tries to bypass the scheduler and emit an event synchronously, without allowing the run stack to finish first, then because event and dispose can both be called in the same stack, the sink whose constructor is still on the stack can have dispose called before its disposable property has finished being assigned, resulting in the error:

TypeError: Cannot read property 'dispose' of undefined

[briancavalier]

Hey @axefrog. This sounds like a tricky issue. Have you been able to narrow down a case for reproducing it?

"A producer source must never produce an event in the same call stack as its run method is called..."

The holdAdd() method is currently violating this policy

The key is the qualifier "producer". A producer is a specific kind of source that synthesizes events. Typically, a producer implements only Source, as opposed to a transform, like map, which implements both Source and Sink. Only producers are subject to the call stack restriction.

hold is a derivative of multicast, which implements both Source and Sink (in the same prototype, in this case). So, multicast isn't a producer, since it doesn't synthesize new events. I guess hold might be slightly more of a gray area: maybe it can be viewed as synthesizing a new event from the value of a previously seen event, but at a new time.

Anyway, the point about letting the run stack settle is a good one. I'd like to dig into it if we can find a reproducible case.

Also: What version of @most/multicast are you using?

[axefrog]

I think this goes back to the discussion we've had in the past regarding events which are replays of past emissions. Your feeling, as I understood it, was that such events aren't normal events and thus should retain original values of t and so forth. I recall that you changed your mind about that position with respect to some subsequent changes you made; I think this case falls under the same banner of expected behaviour. In spirit, yes, we're replaying a past event, but I think the following points are more important:

1. The fact that the event was a "past" event is not relevant to a new consumer because the old value of t is never passed to the consumer, hence t is an implementation detail and only relevant in terms of the way it is used by the scheduler.
2. When a new consumer is added to hold via multicast, it is entirely a new run stack. It may already be active, sure, but as I understand it, the first call must be asynchronous in order to allow the sink chain to finish constructing before it is used and thus potentially disposed. A secondary consumer added to a multicast group does not know or care about its fellow consumers. In that context, it is a new run stack setting up a new sink chain for a new consumer. It can't be expected to behave correctly if an event is emitted before the sink chain has finished being constructed.

My dependency on multicast is implicit via most and @most/hold - 1.1.4

[RamIdeas]

I just ran into the TypeError: Cannot read property 'dispose' of undefined as described by @axefrog

I am able to reproduce the error with the following simplified case:

it( '.take() after hold', async () => {
    const stream = most.just().thru( hold );
    const latest = most.periodic(1).flatMap( () => stream.take(1) );
    await latest.drain();
});

[RamIdeas]

@axefrog do you have a branch somewhere using scheduler.asap?

Is it just a case of changing the line you mentioned to this?

scheduler.asap( PropagateTask.event(this._hold.value, sink) );

(Obviously once refactoring to bring scheduler Into scope)

Update:

I tried this but it made the 2nd test fail with: AssertionError: [ 0, 1, 2 ] deepEqual [ 1, 2, 0 ].

[axefrog]

@RamIdeas A branch... of most-test?

[RamIdeas]

Sorry, meant fork... of mostjs/hold

But I tried it myself in the end. It didn't allow the tests to pass - which is fine because the semantics have been changed. But, weirdly, the results were non-deterministic:

Sometimes the order events are received in is [ 1, 2, 0 ] and sometimes it's [ 1, 0, 2 ].

[RamIdeas]

I've described my intentions here - do either of you have any ideas?

[briancavalier]

Fixed by #25 in 3.0.1