-
Notifications
You must be signed in to change notification settings - Fork 319
feat(watch): add Watch.Watcher#cancel to cancel a single watcher #1518
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
feat(watch): add Watch.Watcher#cancel to cancel a single watcher #1518
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: chaofengliu-okg The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Signed-off-by: chaofengliu-okg <[email protected]>
Signed-off-by: chaofengliu-okg <[email protected]>
Signed-off-by: chaofengliu-okg <[email protected]>
… order Signed-off-by: chaofengliu-okg <[email protected]>
7b2f9c3 to
6ea9fe4
Compare
Signed-off-by: chaofengliu-okg <[email protected]>
11ed645 to
738b55c
Compare
|
should this be part of |
|
If we alter the close implementation, could this break existing user dependencies on the current close behavior? |
|
I think this is a leftover from the original design where we were supposed to re-use the same stream across multiple watcher, but this is not the case as today, so I'd for merging the functions. |
Signed-off-by: chaofengliu-okg <[email protected]>
Signed-off-by: chaofengliu-okg <[email protected]>
Signed-off-by: chaofengliu-okg <[email protected]>
ff4c9a6 to
aa578a9
Compare
|
I've changed the modification status from 'cancel' to 'close'. Please review again. |
|
@chaofengliu-okg is there any way to add a test for this case ? |
Signed-off-by: chaofengliu-okg <[email protected]>
|
@lburgazzoli I have added the unit tests. |
Signed-off-by: chaofengliu-okg <[email protected]>
|
Thank you for merging the PR. I would like to ask about the schedule for the next release. |
scripts: add Watch.Watcher#cancel to cancel a single watcher
Motivation
Currently, closing a watcher is only possible via
Watcher#close, which merely sends an HTTP/2 END_STREAM frame. However, etcd does not terminate the underlying goroutines and watcher stream in response, which can lead to memory leaks.Solution
By adding a
cancelfunction that directly sends an HTTP/2 RST_STREAM frame, etcd can properly shut down the goroutines and the watcher stream.Reproduction
After Change
After adding the cancel API, replacing
watcher.close()withwatcher.cancel()allows the etcd server to properly release the stream and goroutines.Fixes #1510
@fanminshi @lburgazzoli @vorburger