Skip to content
This repository was archived by the owner on May 24, 2022. It is now read-only.

Conversation

Tbaut
Copy link
Collaborator

@Tbaut Tbaut commented Jan 22, 2019

  • closes: Showing "not connected to any peer" with 10/50 and syncing #369
  • The syncStatus$ was emitting several isSync: false more frequently than 2s because the blocks (also part of syncStatus$) were changing. This prevented rpc$ to be emitting and the UI to be updated.
  • audit allows to delay the emission of an event of 2s when not synced. The down side of this solution is that when syncing we only get the sync rate every 2s.
  • all and all what changed is L115, my linter makes it look bigger than it is.

})
// Emit "not synced" only if we haven't been synced for over 2 seconds
)
.pipe(audit(syncStatus => interval(syncStatus.isSync ? 0 : 2000))),
Copy link
Collaborator

Choose a reason for hiding this comment

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

hmm, I don't get the difference between before and after

Before: on each syncStatus update, do:

  • if we're synced, then show that we're synced
  • if we're syncing, then wait 2s, then show that we're syncing (if we're still syncing after those 2s).

And new version: on each syncStatus update, do:

  • wait 0s if we were synced, then show syncStatus
  • wait 2s if we were syncing, then show syncStatus

Maybe I don't understand audit enough. Does it play the same role as delay here?

ps: would appreciate @axelchalon's eyes on this too

Copy link
Collaborator Author

@Tbaut Tbaut Jan 23, 2019

Choose a reason for hiding this comment

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

Before: on each syncStatus update, do:

  • if we're synced, then show that we're synced
  • if we're syncing, then wait 2s to emit. The emission is reset if syncStatus updates in the mean time (that's the problem).

And new version: on each syncStatus update, do:

  • wait 0s if we were synced, then show syncStatus
  • wait 2s if we were syncing, then show the most recent syncStatus value.

The main difference is that audit will emit anyway (with the most recent value), delay will not necessarily emit. Note that throttle sits in between and would emit anyway, but with a not up to date value.

BTW I have to thank @axelchalon who helped me debug and find this solution 💚

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah nice! got it

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, the root of the problem was the reasoning

// syncStatus$() is distinctUntilChanged, so {isSync: false} will never be fired twice in a row.

which was false, because syncStatus$() returns {isSync: false, currentBlock: ..., highestBlock: ..., startingBlock: ...} so even though it's distinct until changed it might emit {isSync: false, ...} multiple times in a row

@Tbaut Tbaut requested a review from axelchalon January 23, 2019 13:21
Copy link
Contributor

@axelchalon axelchalon left a comment

Choose a reason for hiding this comment

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

Nice job investigating this! 🎉

@axelchalon axelchalon merged commit 1fd1b98 into master Jan 23, 2019
@amaury1093 amaury1093 deleted the tbaut-health-status-fix branch January 23, 2019 16:35
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Showing "not connected to any peer" with 10/50 and syncing
3 participants