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

Set ChainService process_block channel size to zero #4418

Merged
merged 1 commit into from
Apr 19, 2024

Conversation

eval-exec
Copy link
Collaborator

@eval-exec eval-exec commented Apr 14, 2024

What problem does this PR solve?

On the develop branch, the ChainService's process_block channel has a capacity of DEFAULT_CHANNEL_SIZE(32), but it does not provide any benefit for ckb-sync or ckb-chain:

let (process_block_sender, process_block_receiver) = channel::bounded(DEFAULT_CHANNEL_SIZE);

In a scenario where ckb-sync sends a block to ChainService through ChainController::process_block, and at the same time, ChainService receives a Ctrl-C exit signal.
Then ChainService's process_block_sender have one ProcessBlockRequest in its buffer:

type ProcessBlockRequest = Request<(Arc<BlockView>, Switch), Result<bool, Error>>;

When self.process_block_sender and self.stop_receiver are ready at the same time, ChainService's select! may choose to execute self.stop_receiver:

ckb/chain/src/chain.rs

Lines 237 to 272 in 149b5c5

select! {
recv(process_block_receiver) -> msg => match msg {
Ok(Request { responder, arguments: (block, verify) }) => {
let instant = Instant::now();
let _ = tx_control.suspend_chunk_process();
let _ = responder.send(self.process_block(block, verify));
let _ = tx_control.continue_chunk_process();
if let Some(metrics) = ckb_metrics::handle() {
metrics
.ckb_block_process_duration
.observe(instant.elapsed().as_secs_f64());
}
},
_ => {
error!("process_block_receiver closed");
break;
},
},
recv(truncate_receiver) -> msg => match msg {
Ok(Request { responder, arguments: target_tip_hash }) => {
let _ = tx_control.suspend_chunk_process();
let _ = responder.send(self.truncate(&target_tip_hash));
let _ = tx_control.continue_chunk_process();
},
_ => {
error!("truncate_receiver closed");
break;
},
},
recv(signal_receiver) -> _ => {
info!("ChainService received exit signal, exit now");
break;
}
}

then the process_block_receiver will be dropped.

However, the process_block_sender is held by the Synchronizer's ChainController, and there is one ProcessBlockRequest in process_block_sender's buffer, causing the ChainController::internal_block_process to block indefinitely.

In that case, Synchronizer::received will block indefinitely at this point:

tokio::task::block_in_place(|| self.process(nc.as_ref(), peer_index, msg));

What is changed and how it works?

Related changes

  • Set ChainService's process_block channel capacity size to 0.

Check List

Tests

  • Unit test
  • Integration test
  • Manual test (add detailed scripts or steps below)
  • No code ci-runs-only: [ quick_checks,linters ]

Side effects

  • None

Release note

Title Only: Include only the PR title in the release note.

@eval-exec eval-exec requested a review from a team as a code owner April 14, 2024 16:49
@eval-exec eval-exec requested review from zhangsoledad and removed request for a team April 14, 2024 16:49
@eval-exec eval-exec added the m:sync module: ckb-sync label Apr 15, 2024
@doitian
Copy link
Member

doitian commented Apr 17, 2024

Will it have any negative impact?

@eval-exec
Copy link
Collaborator Author

Will it have any negative impact?

There should be no negative impact.

@driftluo
Copy link
Collaborator

ref:crossbeam-rs/crossbeam#1102
this is a bug for crossbeam channel

@zhangsoledad zhangsoledad added this pull request to the merge queue Apr 19, 2024
Merged via the queue into nervosnetwork:develop with commit 33697da Apr 19, 2024
32 checks passed
@doitian doitian mentioned this pull request Apr 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
m:sync module: ckb-sync
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants