Implement error codes spec #2927
Conversation
sukunrt
force-pushed
the
sukun/error-codes
branch
3 times, most recently
from
a6e85e0 to
b7d79b0
Compare
sukunrt
force-pushed
the
sukun/error-codes
branch
2 times, most recently
from
66c8cdb to
53d436f
Compare
sukunrt
force-pushed
the
sukun/error-codes
branch
4 times, most recently
from
f60e35b to
3d1038b
Compare
sukunrt
force-pushed
the
sukun/error-codes
branch
from
3d1038b to
aa8b049
Compare
sukunrt
force-pushed
the
sukun/error-codes
branch
from
d6baf8e to
1ad628f
Compare
| @@ -61,6 +101,13 @@ type MuxedStream interface { | |||
| SetWriteDeadline(time.Time) error | |||
| } | |||
|
|
|||
| type ResetWithErrorer interface { | |||
There was a problem hiding this comment.
Why not add this to the MuxedStream Interface? I'm worried that this approach will have the following issues:
- Every usage will need to do an interface check to do this.
- If we wrap the stream or conn, we could easily forget to also implement these interfaces and silently drop this ability.
|
|
||
| // ResetWithError closes both ends of the stream with errCode. The errCode is sent | ||
| // to the peer. | ||
| ResetWithError(errCode StreamErrorCode) error |
There was a problem hiding this comment.
What about CloseWithError as well?
| if errors.As(err, &ce) { | ||
| return &network.ConnError{Remote: ce.Remote, ErrorCode: network.ConnErrorCode(ce.ErrorCode)} | ||
| } | ||
| // TODO: How should we handle resets for reason other than a remote error |
There was a problem hiding this comment.
I don't understand this TODO, can you elaborate?
| @@ -33,6 +33,14 @@ func (c *tconn) Close() error { | |||
| return nil | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
Can we add a test here that the remote gets the expected error back?
| @@ -144,6 +144,22 @@ func (s *stream) Reset() error { | |||
| return nil | |||
| } | |||
|
|
|||
| func (s *stream) ResetWithError(errCode network.StreamErrorCode) error { | |||
| // Cancel any pending reads/writes with an error. | |||
| // TODO: Should these be the other way round(remote=true)? | |||
There was a problem hiding this comment.
Yes. The errors will be returned to the other side of the pipe. In this case the peer will see these errors, so we should set remote=true.
| return | ||
| } | ||
| _, err = s.Read(b) | ||
| fmt.Println(err) |
| err = s.ResetWithError(42) | ||
| require.NoError(t, err) | ||
|
|
||
| _, err = s.Read(buf) |
There was a problem hiding this comment.
How common is it to read/write to a stream after resetting it? That seems like a logic error to me.
| s, err := client.NewStream(ctx, server.ID(), "/test") | ||
| require.NoError(t, err) | ||
|
|
||
| _, err = s.Write([]byte("hello")) |
There was a problem hiding this comment.
nit: It seems like all these tests do this simple echo. Can we move that logic to a separate fn and have the test code be a bit smaller?
| if errors.As(err, &se) { | ||
| code := se.ErrorCode | ||
| if code > math.MaxUint32 { | ||
| // TODO(sukunrt): do we need this? |
There was a problem hiding this comment.
This should probably set the code to a sentinel value that signifies there was an error converting the transport code to a libp2p code. We also need to update the specs to mention this case (in case we haven't).
| return s.closeRead(0, false) | ||
| } | ||
|
|
||
| func (s *stream) closeRead(errCode network.StreamErrorCode, remote bool) error { | ||
| s.mx.Lock() | ||
| defer s.mx.Unlock() | ||
| var err error | ||
| if s.receiveState == receiveStateReceiving && s.closeForShutdownErr == nil { | ||
| err = s.writer.WriteMsg(&pb.Message{Flag: pb.Message_STOP_SENDING.Enum()}) |
There was a problem hiding this comment.
Isn't this missing the error code in the pb Message? Why didn't the transport test catch this?
Specification: libp2p/specs#479