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

fix: Fix bug #355

Merged
merged 4 commits into from
Jun 22, 2022
Merged

fix: Fix bug #355

merged 4 commits into from
Jun 22, 2022

Conversation

driftluo
Copy link
Collaborator

@driftluo driftluo commented Jun 20, 2022

In the process of upgrading the ckb dependency, I encountered the problem that the unit test did not pass after the upgrade, and after spending about two days locating and fixing it, I produced this PR.

First of all, the problem arises when the upgrade process incorrectly implements a trait, which happens with ckb's ping protocol:

    /// Behave like `Stream::poll_next`, but nothing output
    /// if ready with Some, it will continue poll immediately
    /// if ready with None, it will don't try to call the function again
    #[inline]
    async fn poll(&mut self, _context: ProtocolContextMutRef<'_>) -> Option<()> {
        Some(())
    }

This implementation causes the future generated by this method to be called uninterruptedly, which is not a big problem when a runtime starts a network, but when multiple networks are created at the same time, all workers will be stuck in this future and other tasks will not be executed, which causes the unit tests of the network module to fail extensively, but the integration tests pass entirely.

In rust's collaborative asynchronous model, the problem of user implementation errors is hard to cure, but as a framework, we have to make the implementation work as well as possible, so I introduced tokio's budget model, which takes the framework's controllable future and adds the logic of forced yield to make it work as well as possible even in exception states. Since we don't have a need to disable it, the implementation is simpler than tokio.

The ckb network unit test passes 100% even in the bad implementation(ping protocol impl with Some(())).

There are many ways to implement the budget.

  1. inline it into the implementation of the specified Future, which causes each future to keep track of its own execution times
  2. introduce budget as a CPU-like time slice, i.e. the current implementation, which is less intrusive and can be shared externally, which is how tokio is implemented, but it does not expose the calling interface for now(unstable with consume_budget, but it can't use on poll fn)

Another bug fixed in the implementation of poll fn, which does not call it if it returns none, as the documentation says

Remove the external exposure of socket2, so that external interaction is only possible via fd

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

Successfully merging this pull request may close these issues.

None yet

2 participants