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.

Sign up for GitHub

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

test/gracefulstop: use stubserver instead of testservice implementation #7907

Open
wants to merge 9 commits into
base: master
Choose a base branch
from
Open

test/gracefulstop: use stubserver instead of testservice implementation #7907

Changes from 1 commit
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
6da62e6
switching to stubserver
pvsravani Dec 6, 2024
a73e23c
switching to stubserver
pvsravani Dec 9, 2024
0bfd0f6
Removing redundant server and updating mutex in close()
pvsravani Dec 16, 2024
3d059c2
reverting mutex and closed fields
pvsravani Dec 17, 2024
a623c8a
Moving the intialization of S field into the stubserver
pvsravani Dec 17, 2024
61374bc
renaming d as dialer and removing sync.waitgroup()
pvsravani Dec 27, 2024
1df0441
Renaming done as gracefulStopDone
pvsravani Jan 8, 2025
f976afd
Renaming done as gracefulStopDone
pvsravani Jan 8, 2025
2190f99
Moving <-dlis.acceptCalled out of go routine
pvsravani Jan 8, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Renaming done as gracefulStopDone
  • Loading branch information
@pvsravani
pvsravani committed Jan 8, 2025
commit 1df0441c51d0542d49816c54796f0bc0bec068b6
10 changes: 6 additions & 4 deletions test/gracefulstop_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -124,13 +124,15 @@ func (s) TestGracefulStop(t *testing.T) {
stubserver.StartTestService(t, ss)
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this is doing the correct thing. What we want as part of this test is for GracefulStop to be called when Accept is in the middle of accepting a connection.

If you folks have convinced yourself that it actually does what it is supposed to do, then we don't need a sync.WaitGroup. The same can be accomplished with a done channel.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed sync.WaitGroup

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah i don't think we can use sync.waitgroup with stubserver.StartService(). stubserver.StartTestService creates a server and starts serving in the background. It handles server lifecycle internally. Having a done channel to track gracefulstop will work correctly.


// 1. Start Server
done := make(chan struct{})
gracefulStopDone := make(chan struct{})
<-dlis.acceptCalled
ss.S.GracefulStop()
close(gracefulStopDone)
go func() {
<-dlis.acceptCalled
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think its needed to move it within go routine. Not that its wrong but this change is not needed i think.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

ss.S.GracefulStop()
close(done)
close(gracefulStopDone)
}()

// 2. GracefulStop() Server after listener's Accept is called, but don't
// allow Accept() to exit when Close() is called on it.

Expand Down Expand Up @@ -159,7 +161,7 @@ func (s) TestGracefulStop(t *testing.T) {
t.Fatalf("FullDuplexCall= _, %v; want _, <status code Unavailable>", err)
}
cancel()
<-done
<-gracefulStopDone
}

// TestGracefulStopClosesConnAfterLastStream ensures that a server closes the
Expand Down
Loading