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

removing deprecated http closenotifier function #6886

Merged
merged 2 commits into from
Dec 21, 2023

Conversation

miledxz
Copy link
Contributor

@miledxz miledxz commented Dec 20, 2023

Regarding this little issue with deprecated function http.CloseNotify,
#6782

I have decided to add just small pr for http.CloseNotify deprecated

In this pull request, I have just removed the implementation, tests are going well,

my other approach was this:

to add and update onlyCloseNotifier interface, by doing this:

type onlyContext {
  http.ResponseWriter
  context.Context
}

then to update whole testHandlerResponseWriter with corresponding context.Context methods, like these:

type testHandlerResponseWriter struct {
    *httptest.ResponseRecorder
    done chan struct{}
    err error
    value any
    deadline time.Time
    ok bool
}

func (w testHandlerResponseWriter) Deadline() (deadline time.Time, ok bool) {return time.Time{}, false}
func (w testHandlerResponseWriter) Done() <-chan struct{}                              { return w.Result().Request.Context().Done() }
func (w testHandlerResponseWriter) Err() error                                                    {return nil}
func (w testHandlerResponseWriter) Value(key any) any                                     { return nil}

testHandlerResponseWriter{
        ResponseRecorder: httptest.NewRecorder(),
        done: make(chan struct{}, 1),
        err: nil,
        value: nil,
        deadline: time.Time{},
        ok: true,
}

But I suppose non of this is required ?

RELEASE NOTES: none

Copy link

linux-foundation-easycla bot commented Dec 20, 2023

CLA Signed

The committers listed above are authorized under a signed CLA.

@zasweq
Copy link
Contributor

zasweq commented Dec 20, 2023

Hello, as I mentioned in the issue, we currently cannot take this change. If this change was taken, we would need to do it cross language. Hence, can you please file an issue at the link I listed in the issue you brought up. Thank you!

@zasweq zasweq closed this Dec 20, 2023
@dfawley
Copy link
Member

dfawley commented Dec 20, 2023

I think this was the wrong PR? @zasweq ?

@dfawley dfawley reopened this Dec 20, 2023
Copy link

codecov bot commented Dec 20, 2023

Codecov Report

Merging #6886 (b31db19) into master (bb0d32f) will increase coverage by 0.13%.
The diff coverage is n/a.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #6886      +/-   ##
==========================================
+ Coverage   83.65%   83.78%   +0.13%     
==========================================
  Files         286      286              
  Lines       30756    30756              
==========================================
+ Hits        25730    25770      +40     
+ Misses       3963     3936      -27     
+ Partials     1063     1050      -13     

see 19 files with indirect coverage changes

@dfawley
Copy link
Member

dfawley commented Dec 20, 2023

But I suppose non of this is required ?

Yeah...it looks like nothing ever read from the notifier channel? SGTM as long as all the tests pass!

In the meantime, there is a lint error (just need to run gofmt):

https://github.com/grpc/grpc-go/actions/runs/7282068214/job/19843764206?pr=6886#step:7:259

@miledxz
Copy link
Contributor Author

miledxz commented Dec 21, 2023

@dfawley , yes, thats exactly what I meant, notifier channel seems not to be required,
I have added update,

thank you for time and understanding with opening issue so we can proceed

@zasweq
Copy link
Contributor

zasweq commented Dec 21, 2023

Oh whoops sorry wrong PR

@dfawley
Copy link
Member

dfawley commented Dec 21, 2023

+@easwars for a second pass

@easwars easwars merged commit 4f03f3f into grpc:master Dec 21, 2023
14 checks passed
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 19, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants