Server should send RST_STREAM when deadline is exceeded #2
Conversation
internal/transport/http2_server.go
Outdated
| @@ -1104,6 +1104,9 @@ func (t *http2Server) WriteStatus(s *Stream, st *status.Status) error { | |||
| } | |||
| // Send a RST_STREAM after the trailers if the client has not already half-closed. | |||
| rst := s.getState() == streamActive | |||
| if st.Code() == codes.DeadlineExceeded { | |||
There was a problem hiding this comment.
can you write a test for this or modify an exsting test to verify?
There was a problem hiding this comment.
How did you verify if this is the only change we need? Did you test we are actually getting RST_STREAM at client?
There was a problem hiding this comment.
@purnesh42H Please find the screen once putting the sleep time 40sec, deadline exceed we are getting if we are remove the check "context cancel" respectively in the flow_control feature example 
There was a problem hiding this comment.
you need to write a test to verify where client is receiving RST_STREAM when deadline exceeded. Check existing tests where RST_STREAM is sent
There was a problem hiding this comment.
sure @purnesh42H I will do same
internal/transport/http2_server.go
Outdated
| @@ -1042,6 +1042,18 @@ func (t *http2Server) writeHeaderLocked(s *Stream) error { | |||
| return nil | |||
| } | |||
|
|
|||
| type NoopFunc func() | |||
|
|
|||
| var noop NoopFunc = func() { | |||
There was a problem hiding this comment.
please see how other noop functions are implemented. There are some for goaway tests. Please make a habit of going through existing code to get an idea.
Give meaningful name to it. May be something like SignalDeadlineExceededForTesting. Also, it should not be exported.
internal/transport/http2_server.go
Outdated
| @@ -1042,6 +1042,18 @@ func (t *http2Server) writeHeaderLocked(s *Stream) error { | |||
| return nil | |||
| } | |||
|
|
|||
| type NoopFunc func() | |||
internal/transport/http2_server.go
Outdated
| // Default noop function does nothing | ||
| } | ||
|
|
||
| func setRstAndCallNoop(rst *bool) { |
There was a problem hiding this comment.
this is not needed either. Please remove
internal/transport/http2_server.go
Outdated
| @@ -1104,6 +1116,10 @@ func (t *http2Server) WriteStatus(s *Stream, st *status.Status) error { | |||
| } | |||
| // Send a RST_STREAM after the trailers if the client has not already half-closed. | |||
| rst := s.getState() == streamActive | |||
| if st.Code() == codes.DeadlineExceeded { | |||
| rst = true | |||
| setRstAndCallNoop(&rst) | |||
There was a problem hiding this comment.
you should just call the noop function here which is overriden in the test
internal/transport/transport_test.go
Outdated
| ch <- struct{}{} | ||
| } | ||
| var rst bool | ||
| setRstAndCallNoop(&rst) |
There was a problem hiding this comment.
this has to be declared in the test. Please see resolver tests how things are being done there. You need to also assign it back to original definition
misvivek
force-pushed
the
rst-stream-on-deadline-exceed
branch
from
8bdfb99 to
5ce3627
Compare
Problem:
#issue no: 2886
Server should send RST_STREAM when deadline is exceeded
Action:
Added the condition for RST_STREAM once status code matches
Release note:
NA