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

Close ChainNotify channel when it blocks #6947

Closed
4 tasks done
Stebalien opened this issue Jul 30, 2021 · 4 comments
Closed
4 tasks done

Close ChainNotify channel when it blocks #6947

Stebalien opened this issue Jul 30, 2021 · 4 comments
Assignees
Labels
kind/enhancement Kind: Enhancement P2 P2: Should be resolved

Comments

@Stebalien
Copy link
Member

Checklist

  • This is not a new feature or an enhancement to the Filecoin protocol. If it is, please open an FIP issue.
  • This is not a new feature request. If it is, please file a feature request instead.
  • This is not brainstorming ideas. If you have an idea you'd like to discuss, please open a new discussion on the lotus forum and select the category as Ideas.
  • I have a specific, actionable, and well motivated improvement to propose.

Lotus component

Other

Improvement Suggestion

Currently, if a caller fails to read from the ChainNotify channel, everything will block until they do. This means one slow caller can slow everything else down and cause problems for the entire node (see #6883).

Proposal: If writing to the ChainNotify channel would block (caller isn't reading), close the channel. When the caller resumes reading, they'll see the close, know that they skipped some events, re-subscribe, then figure out which events they missed. Effectively, the caller will just "restart" itself.

@Stebalien Stebalien self-assigned this Jul 30, 2021
@Stebalien
Copy link
Member Author

cc @magik6k any good reasons not to at least try this?

@Stebalien
Copy link
Member Author

A (simpler) alternative solution would be to build the "catchup" part into SubHeadChanges itself.

  1. If writing to the output channel blocks (in SubHeadChanges), start dropping notifications (but keep the first "unsent" and last seen notification).
  2. If the output channel gets unblocked, walk the dropped part of the chain, sending notifications for each entry.

In terms of efficiency, it's probably better to just close the channel. However, that's a significantly more invasive change.

@jennijuju jennijuju added the P2 P2: Should be resolved label Aug 2, 2021
@Stebalien
Copy link
Member Author

I've gone with the first option and tests seem to pass.

@Stebalien
Copy link
Member Author

Fixed in #7000.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/enhancement Kind: Enhancement P2 P2: Should be resolved
Projects
None yet
Development

No branches or pull requests

3 participants