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

fix: smoldot connection race condition #671

Merged
merged 6 commits into from
Feb 28, 2023
Merged

fix: smoldot connection race condition #671

merged 6 commits into from
Feb 28, 2023

Conversation

kratico
Copy link
Contributor

@kratico kratico commented Feb 27, 2023

  • Fixes subscription handler setup (Connection.handle was invoked before the subscription handler was set)
  • Add unit test for Smoldot

@kratico kratico marked this pull request as ready for review February 28, 2023 13:36
@kratico kratico marked this pull request as draft February 28, 2023 13:36
@kratico kratico marked this pull request as ready for review February 28, 2023 14:43
harrysolovay
harrysolovay previously approved these changes Feb 28, 2023
Copy link
Contributor

@harrysolovay harrysolovay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

delete this.subscriptionHandlers[subscriptionId]
this.send(this.nextId++, unsubscribe, [subscriptionId])
})
const id = this.nextId++
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why were these changes to subscription reverted?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kratico found a bug in which the subscription handler is not set before the first message is received. Was specific to Smoldot if I'm not mistaken.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@harrysolovay @tjjfvi this is surfaced with smoldot because it Connection.handle is invoked before the subscription handler is set.
It may happen with WebSockets too but, most of the time, the first subscription message in WS is not very fast as in smoldot.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, does smoldot send multiple messages synchronously? In that case, it wouldn't be a question of speed, just the microtask from the await.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe that for this particular subscription, the chainHead_unstable_follow responds with 2 messages very quickly

  • the response for the subscription id
  • the response for the first subscription message

It's a microtask race condition

For the above subscription, this smoldot loop run 2 times

    while (true) {
      try {
        const response = await Promise.race([
          this.listening,
          chain.nextJsonRpcResponse(),
        ])
        if (!response) break
        this.handle(JSON.parse(response))
      } catch (_e) {}
    }

before this connection.subscription

    const message = await this.#call(id, subscribe, params)
    if (signal.aborted) {
      delete this.subscriptionPendingInits[id]
      return
    }
    if (message.error) {
      delete this.subscriptionPendingInits[id]
      return handler(message)
    }

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah yes, I believe this is because deferred.resolve uses two microtask ticks (first to resolve the argument, then to resolve the real promise). I plan to report this to deno std (this, along with another related issue with deferred), but this is a good solution for now.

@harrysolovay harrysolovay changed the title feat: revisit smoldot integration fix: smoldot connection race condition Feb 28, 2023
@harrysolovay harrysolovay merged commit 76ba340 into main Feb 28, 2023
@harrysolovay harrysolovay deleted the feat/smoldot branch February 28, 2023 19:57
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants