Skip to content
This repository was archived by the owner on Sep 30, 2024. It is now read-only.

feat(sg): respect the context when executing interrupt hooks#63069

Merged
burmudar merged 7 commits into
mainfrom
wb/sg-interrupts
Jun 5, 2024
Merged

feat(sg): respect the context when executing interrupt hooks#63069
burmudar merged 7 commits into
mainfrom
wb/sg-interrupts

Conversation

@burmudar

@burmudar burmudar commented Jun 4, 2024

Copy link
Copy Markdown
Contributor

During testing I found that sometimes some hooks would just hang and not complete. In this PR we execute all hooks within a timeout context. Ensuring we give some time for hooks to execute but also making sure we eventually exit if some hook is misbehaving.

Additional changes:

  • Global timeout for all hook execution is 2 seconds
  • We hard exit after 5 intterupts instead of 2
  • Hooks are split into two groups: sequential and concurrent. As per their names the hooks are executed differently depending how they were registered.

Test plan

Tested locally

^C⚠️ Interrupt received, executing hook groups for graceful shutdown...
⚠️ Executing 16 'cleanup' hooks for graceful shutdown...
[   repo-updater] INFO repo-updater.repo-updater.grpcserver grpcserver/grpcserver.go:76 Shutting down gRPC server
[   repo-updater] INFO sync_worker workerutil/worker.go:252 Shutting down dequeue loop {"name": "repo_sync_worker", "reason": ""}
worker stopped due to context error: context canceled
gitserver-1 stopped due to context error: context canceled
searcher stopped due to context error: context canceled
gitserver-0 stopped due to context error: context canceled
blobstore stopped due to context error: context canceled
symbols stopped due to context error: context canceled
caddy stopped due to context error: context canceled
repo-updater stopped due to context error: context canceled
embeddings stopped due to context error: context canceled
frontend stopped due to context error: context canceled
zoekt-index-0 stopped due to context error: context canceled
syntax-highlighter stopped due to context error: context canceled
zoekt-web-1 stopped due to context error: context canceled
web stopped due to context error: context canceled
zoekt-web-0 stopped due to context error: context canceled
⚠️ Executing 6 'general' hooks for for graceful shutdown...
❌ failed to run zoekt-index-1.
stderr:
INFO server zoekt-sourcegraph-indexserver/main.go:1017 removing tmp dir {"tmpRoot": "/Users/william/.sourcegraph/zoekt/index-1/.indexserver.tmp"}
2024/06/04 09:15:03 updating index 6 github.com/sourcegraph/sourcegraph@HEAD=e55003da894490122546f876452f651aae65bb55 reason=content-mismatch
INFO server zoekt-sourcegraph-indexserver/main.go:432 updated index {"repo": "github.com/sourcegraph/sourcegraph", "id": 6, "branches": ["HEAD=e55003da894490122546f876452f651aae65bb55"], "duration": "19.21403925s"}

Changelog

  • Hard exit sg when 5 intterupt hooks are received
  • Respect the context while executing interrupt hooks to ensure we still exit if some hook is misbehaving

burmudar added 3 commits June 3, 2024 15:45
@burmudar
sg: group interrupts
f90cd67
@burmudar
remove debugging
1fc1dc2
@burmudar
sg: rework interrupts to respect contexts
d8bf9bc 
Intterupts hook are now split into two groups:
- Sequential
- Concurrent

When hooks are executed now within a timeout context. Ensuring that we
_still_ exit even if some hooks take too long.

Also change max amount of interrupts signals we handle to be 5 since 2
felt a bit too short.
@burmudar burmudar requested a review from eseliger June 4, 2024 09:20
@burmudar burmudar self-assigned this Jun 4, 2024
@cla-bot cla-bot Bot added the cla-signed label Jun 4, 2024
@burmudar burmudar requested a review from a team June 4, 2024 09:25
@eseliger

eseliger commented Jun 4, 2024

Copy link
Copy Markdown
Member

Ensuring we give some time for hooks to execute but also making sure we eventually exit if some hook is misbehaving.

Isn't that why the 2nd sigint from the user terminates immediately using SIGKILLs?
Should we not trust the hooks for the regular exit mode?

We hard exit after 5 intterupts instead of 2

Why is that?

@burmudar

burmudar commented Jun 4, 2024 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Ensuring we give some time for hooks to execute but also making sure we eventually exit if some hook is misbehaving.

Isn't that why the 2nd sigint from the user terminates immediately using SIGKILLs? Should we not trust the hooks for the regular exit mode?

No - unfortunately not. That would be the more correct way to do it but a hook currently has no knowledge what signal was sent nor how many times a signal has been sent. We execute the same hooks regardless how many signals were sent 😬 So currently we handle it higher up since 99% of the use cases are people smashing ctrl-c - once we see that you're probably pressing ctrl-c a bunch of times we take it to mean "for the love of god stop"

We hard exit after 5 intterupts instead of 2

Why is that?

This is more as a failsafe. Technically now with the context timeout do not need to do this since we will exit but if something goes wrong there then ... smashing ctrl-c will still make sg exit and not hang around

jamesmcnamara
jamesmcnamara approved these changes Jun 4, 2024
View reviewed changes

@jamesmcnamara jamesmcnamara left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I like the opt-in for concurrent execution!

burmudar
burmudar commented Jun 4, 2024
View reviewed changes
Comment thread dev/sg/interrupt/interrupt.go Outdated
@burmudar burmudar merged commit e4eec66 into main Jun 5, 2024
@burmudar burmudar deleted the wb/sg-interrupts branch June 5, 2024 08:06
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@eseliger eseliger Awaiting requested review from eseliger

1 more reviewer

@jamesmcnamara jamesmcnamara jamesmcnamara approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@burmudar burmudar

Labels

cla-signed

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@burmudar @eseliger @jamesmcnamara