Skip to content

Conversation

abhinav
Copy link
Collaborator

@abhinav abhinav commented May 6, 2023

Stacked on top of:

However, since I can't push branches directly to this repository,
this PR shows commits from both PRs.


Shutdowner.Shutdown is not a blocking operation.
It does not need to block and wait for the relay goroutine to stop;
signalReceiver.Stop will do that.

To clarify this further, signalReceivers has the following operations:

  • Start: starts a relay goroutine
  • Stop: stops the relay goroutine and waits for it
  • Wait: create and return a chan ShutdownSignal
  • Done: create and return a chan os.Signal
  • Broadcast: Send the message to all waiting channels

The only reason that the relay goroutine exists is
to map os.Signals received by os/signal.Notify
into an fx.ShutdownSignal.

Shutdowner.Shutdown should not call signalReceivers.Stop
because the relay goroutine should keep running
so that we can re-use Shutdowner.Shutdown.

This change exposed a leak in TestDataRace (fixed in #1081).

abhinav added 2 commits May 6, 2023 15:31
TestDataRace never calls App.Stop, which can cause a resource leak.
This currently passes because of the implementation detail that
Shutdowner.Shutdown happens to call signalReceivers.Stop
instead of just Shutdown, which may not always be true.
Shutdowner.Shutdown is not a blocking operation.
It does not need to block and wait for the relay goroutine to stop;
signalReceiver.Stop will do that.

To clarify this further, signalReceivers has the following operations:

- Start: starts a relay goroutine
- Stop: stops the relay goroutine and waits for it
- Wait: create and return a `chan ShutdownSignal`
- Done: create and return a `chan os.Signal`
- Broadcast: Send the message to all waiting channels

The only reason that the relay goroutine exists is
to map `os.Signal`s received by `os/signal.Notify`
into an `fx.ShutdownSignal`.

Shutdowner.Shutdown should not call signalReceivers.Stop
because the relay goroutine should keep running
so that we can re-use Shutdowner.Shutdown.

This change exposed a leak in TestDataRace (fixed in uber-go#1081).
@codecov
Copy link

codecov bot commented May 6, 2023

Codecov Report

Merging #1082 (f6e1f57) into master (808956a) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #1082      +/-   ##
==========================================
- Coverage   98.53%   98.52%   -0.01%     
==========================================
  Files          39       39              
  Lines        2997     2984      -13     
==========================================
- Hits         2953     2940      -13     
  Misses         37       37              
  Partials        7        7              
Impacted Files Coverage Δ
shutdown.go 100.00% <100.00%> (ø)
signal.go 100.00% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@sywhang sywhang merged commit ec9b096 into uber-go:master May 8, 2023
sywhang added a commit that referenced this pull request May 8, 2023
Stacked on top of:

- #1081
- #1082

However, since I can't push branches directly to this repository,
this PR shows commits from all PRs.

---

App.Start nils out the "last" signal recorded by signalReceivers,
which it otherwise broadcasts to waiters if it was already received.
This is unnecessary especially because there's a discrepancy in behavior
of using App.Start vs App.Run when shutting down from fx.Invoke.

Given a program that calls Shutdown from fx.Invoke,
when we do:

    app := fx.New(...)

The shutdowner has already sent the signal, and signalReceivers has
already recorded it.
At that point, whether we call App.Start or App.Run changes behavior:

- If we call App.Run, that calls App.Done (or App.Wait after #1075),
  which gives it back a channel that already has the signal filled in.
  It then calls App.Start, waits on the channel--which returns
  immediately--and then calls App.Stop.
- If we call App.Start and App.Wait, on the other hand,
  Start will clear the signal recorded in signalReceivers,
  and then App.Wait will build a channel that will block indefinitely
  because Shutdowner.Shutdown will not be called again.

So even though App.Run() and App.Start()+App.Wait() are meant to be
equivalent, this causes a serious discrepancy in behavior.
It makes sense to resolve this by supporting Shutdown from Invoke.

Refs #1074

---------

Co-authored-by: Sung Yoon Whang <sungyoon@uber.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants