Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Lost events from subscriptions if multiple fire at the same time #402

Closed
davidapgar opened this issue Jun 10, 2019 · 3 comments
Closed

Lost events from subscriptions if multiple fire at the same time #402

davidapgar opened this issue Jun 10, 2019 · 3 comments
Labels
swift Affects the Swift library.
Milestone

Comments

@davidapgar
Copy link
Collaborator

Prior to the concurrency changes in 0.17, if two signals fired at the same time (same runloop), both actions would be received (across two render passes, assuming the subscription was still maintained).

With the most recent changes, only the first subscription's action is received, and the second is silently dropped. This appears to be due to how subscription/signal handling was changed. Prior, it was done through a signal of signals, that were flatMap'd with "latest" and with a constant observation on the output of events. This changed to have the signal subscribe on each pass (after being merged together).

The behavior, at a minimum, is now surprising.

Either

  1. Restore the previous behavior (to not drop events)
    or
  2. assert if multiple signals fire on the same render loop.

If restoring the previous behavior, we must assure that it's the updated action handling - there may have previously been a bug where the second signal firing was handled with the rules of the previous render loop - and thus might have caused an action that wasn't declared (need to investigate this).

@davidapgar
Copy link
Collaborator Author

With an overly specific test case, the previous behavior was a "bug". Example:

Two signals being subscribed to, signalA and signalB, and a state where if we're not "loading" we don't want to subscribe anymore, eg:

enum State {
  case idle
  case loading
}

enum Action {
  case signalFired
  apply() {
    switch self {
      case .signalFired:
        state = .idle
    }
  }
}
render() {
  if state == .loading {
    context.subscribe(signalA.map { return .signalFired })
    context.subscribe(signalB.map { return .signalFired })
  }  
}

Both signalA and signalB fire at the same time (same runloop).
The action from signalA is received, the state updates to .idle. Render happens again, and the signals are not subscribed to.

signalB will still fire with the previous implementation, and the .signalFired action will be handled. This is unexpected behavior, as that signal was not subscribed to after signalA fired.

@davidapgar
Copy link
Collaborator Author

Talked with @zach-klippenstein about how this is handled on the Kotlin side:
(If i understood correctly) Since they are using Channels, events from signals are queued since Channels act as a blocking queue. The mapping from a signal is done after the event is received from a channel.

Thus, the behavior in the example above would be:
signalA and signalB both send events at the same time. The whichever one "wins" (in this example, signalA) gets handled first. The event is mapped to an action and handled.
A render pass happens, and signalB is still subscribed to. Its event is received from the channel, and mapped to the action from the most recent render pass and handled as well.

To add this behavior, we need to:

  1. Change the subscribe interface to have the map to an action be separate from the signal (so the signal sends whatever event/output it wants, and the mapping is defined as a closure in subscribe), eg: func subscribe<Event>(signal: Signal<Event, NoError>, map: (Signal) -> WorkflowAction)
  2. The subscribed signals are mapped based on some unique identifier (possibly the Event, but possibly more constrained to something like the object identifier). When they emit events, they go into a queue.
  3. When any signal emits and event, they queues are checked to see if they have a pending event. The first event is pulled off, and mapped to an action from the above map.
  4. On the subsequent render pass, any signals that were previously subscribed to are kept in the map (that includes their queue). This allows them to be mapped differently on subsequent render passes, but not drop events.

If this is added, a similar behavior should likely be applied to Workers as well (to handled two events on a single render pass)

@dhavalshreyas
Copy link
Contributor

#1048 fixes this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
swift Affects the Swift library.
Projects
None yet
Development

No branches or pull requests

3 participants