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

Support for Pebble Notices #288

Closed
wants to merge 26 commits into from
Closed

Support for Pebble Notices #288

wants to merge 26 commits into from

Conversation

benhoyt
Copy link
Contributor

@benhoyt benhoyt commented Aug 25, 2023

Just a WIP draft at the mojment.

See the spec: JU048.

notice.repeatAfter = repeatAfter

if newOrRepeated {
s.processNoticeWaiters()
Copy link

Choose a reason for hiding this comment

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

it feels a bit dangerous that this can potential block on unrelated code and timeouts decided by it, it would a bit more complex but safer to have in-between goroutines that distribute things to waiters

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The idea here is that processNoticeWaiters can't block, because it only sends to the current noticeWaiters channels, and those are added and removed with each call to WaitNotice (the other goroutines). And we have a done (ctx.Done()) channel on each send too, to avoid blocking if the context is cancelled.

The addNoticeWaiter / removeNoticeWaiter / processNoticeWaiters logic is intentionally very close together in that file, partly for that reason.

So I'm not sure how adding additional in-between goroutines will help here?

@benhoyt
Copy link
Contributor Author

benhoyt commented Sep 19, 2023

Closing this draft in favour of the real PRs, starting with #295 (other PRs linked there).

@benhoyt benhoyt closed this Sep 19, 2023
@benhoyt benhoyt deleted the notices branch September 19, 2023 04:32
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.

2 participants