Skip to content

Commit dfa7e28

Browse files
sukunrtsukunrt
committed
address review comments 2
1 parent 798258c commit dfa7e28

File tree

10 files changed

+72
-33
lines changed

10 files changed

+72
-33
lines changed

‎core/network/conn.go

+3-5
Original file line numberDiff line numberDiff line change
@@ -33,17 +33,14 @@ func (c *ConnError) Error() string {
3333
}
3434

3535
func (c *ConnError) Is(target error) bool {
36-
if target == ErrReset {
37-
return true
38-
}
3936
if tce, ok := target.(*ConnError); ok {
4037
return tce.ErrorCode == c.ErrorCode && tce.Remote == c.Remote
4138
}
4239
return false
4340
}
4441

45-
func (c *ConnError) Unwrap() error {
46-
return c.TransportError
42+
func (c *ConnError) Unwrap() []error {
43+
return []error{ErrReset, c.TransportError}
4744
}
4845

4946
const (
@@ -56,6 +53,7 @@ const (
5653
ConnGarbageCollected ConnErrorCode = 1006
5754
ConnShutdown ConnErrorCode = 1007
5855
ConnGated ConnErrorCode = 1008
56+
ConnCodeOutOfRange ConnErrorCode = 1009
5957
)
6058

6159
// Conn is a connection to a remote peer. It multiplexes streams.

‎core/network/mux.go

+3-5
Original file line numberDiff line numberDiff line change
@@ -32,17 +32,14 @@ func (s *StreamError) Error() string {
3232
}
3333

3434
func (s *StreamError) Is(target error) bool {
35-
if target == ErrReset {
36-
return true
37-
}
3835
if tse, ok := target.(*StreamError); ok {
3936
return tse.ErrorCode == s.ErrorCode && tse.Remote == s.Remote
4037
}
4138
return false
4239
}
4340

44-
func (s *StreamError) Unwrap() error {
45-
return s.TransportError
41+
func (s *StreamError) Unwrap() []error {
42+
return []error{ErrReset, s.TransportError}
4643
}
4744

4845
const (
@@ -55,6 +52,7 @@ const (
5552
StreamGarbageCollected StreamErrorCode = 1006
5653
StreamShutdown StreamErrorCode = 1007
5754
StreamGated StreamErrorCode = 1008
55+
StreamCodeOutOfRange StreamErrorCode = 1009
5856
)
5957

6058
// MuxedStream is a bidirectional io pipe within a connection.

‎p2p/net/connmgr/connmgr_test.go

+4-1
Original file line numberDiff line numberDiff line change
@@ -1009,7 +1009,6 @@ func TestErrorCode(t *testing.T) {
10091009
require.NoError(t, err)
10101010
defer cm.Close()
10111011

1012-
sw1.Notify(cm.Notifee())
10131012
sw1.Peerstore().AddAddrs(sw2.LocalPeer(), sw2.ListenAddresses(), peerstore.PermanentAddrTTL)
10141013
sw1.Peerstore().AddAddrs(sw3.LocalPeer(), sw3.ListenAddresses(), peerstore.PermanentAddrTTL)
10151014

@@ -1039,6 +1038,10 @@ func TestErrorCode(t *testing.T) {
10391038
return true
10401039
}, 10*time.Second, 100*time.Millisecond)
10411040

1041+
not := cm.Notifee()
1042+
not.Connected(sw1, c12)
1043+
not.Connected(sw1, c13)
1044+
10421045
cm.TrimOpenConns(context.Background())
10431046

10441047
require.True(t, c12.IsClosed() || c13.IsClosed())

‎p2p/net/swarm/swarm.go

+8-5
Original file line numberDiff line numberDiff line change
@@ -850,11 +850,14 @@ func (c *connWithMetrics) Close() error {
850850
}
851851

852852
func (c *connWithMetrics) CloseWithError(errCode network.ConnErrorCode) error {
853-
c.metricsTracer.ClosedConnection(c.dir, time.Since(c.opened), c.ConnState(), c.LocalMultiaddr())
854-
if ce, ok := c.CapableConn.(network.CloseWithErrorer); ok {
855-
return ce.CloseWithError(errCode)
856-
}
857-
return c.CapableConn.Close()
853+
c.once.Do(func() {
854+
c.metricsTracer.ClosedConnection(c.dir, time.Since(c.opened), c.ConnState(), c.LocalMultiaddr())
855+
if ce, ok := c.CapableConn.(network.CloseWithErrorer); ok {
856+
c.closeErr = ce.CloseWithError(errCode)
857+
}
858+
c.closeErr = c.CapableConn.Close()
859+
})
860+
return c.closeErr
858861
}
859862

860863
func (c *connWithMetrics) Stat() network.ConnStats {

‎p2p/protocol/circuitv2/relay/relay_test.go

-1
Original file line numberDiff line numberDiff line change
@@ -309,7 +309,6 @@ func TestRelayLimitData(t *testing.T) {
309309

310310
rc := relay.DefaultResources()
311311
rc.Limit.Duration = time.Second
312-
// Due to yamux framing, 4 blocks of 1024 bytes will exceed the data limit
313312
rc.Limit.Data = 4096
314313

315314
r, err := relay.New(hosts[1], relay.WithResources(rc))

‎p2p/test/transport/transport_test.go

+32-7
Original file line numberDiff line numberDiff line change
@@ -869,7 +869,6 @@ func TestConnClosedWhenRemoteCloses(t *testing.T) {
869869
}
870870

871871
func TestErrorCodes(t *testing.T) {
872-
873872
assertStreamErrors := func(s network.Stream, expectedError error) {
874873
buf := make([]byte, 10)
875874
_, err := s.Read(buf)
@@ -925,20 +924,46 @@ func TestErrorCodes(t *testing.T) {
925924
require.NoError(t, err)
926925
pingPong(s)
927926

927+
remoteStream := <-remoteStreamQ
928+
defer remoteStream.Reset()
929+
928930
err = s.ResetWithError(42)
929931
require.NoError(t, err)
930932
assertStreamErrors(s, &network.StreamError{
931933
ErrorCode: 42,
932934
Remote: false,
933935
})
934936

937+
assertStreamErrors(remoteStream, &network.StreamError{
938+
ErrorCode: 42,
939+
Remote: true,
940+
})
941+
})
942+
t.Run("StreamResetWithErrorByRemote", func(t *testing.T) {
943+
if tc.Name == "WebTransport" {
944+
t.Skipf("skipping: %s, not implemented", tc.Name)
945+
return
946+
}
947+
ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second)
948+
defer cancel()
949+
s, err := client.NewStream(ctx, server.ID(), "/test")
950+
require.NoError(t, err)
951+
pingPong(s)
952+
935953
remoteStream := <-remoteStreamQ
936-
defer remoteStream.Reset()
937954

938-
assertStreamErrors(remoteStream, &network.StreamError{
955+
err = remoteStream.ResetWithError(42)
956+
require.NoError(t, err)
957+
958+
assertStreamErrors(s, &network.StreamError{
939959
ErrorCode: 42,
940960
Remote: true,
941961
})
962+
963+
assertStreamErrors(remoteStream, &network.StreamError{
964+
ErrorCode: 42,
965+
Remote: false,
966+
})
942967
})
943968

944969
t.Run("StreamResetByConnCloseWithError", func(t *testing.T) {
@@ -952,6 +977,9 @@ func TestErrorCodes(t *testing.T) {
952977
require.NoError(t, err)
953978
pingPong(s)
954979

980+
remoteStream := <-remoteStreamQ
981+
defer remoteStream.Reset()
982+
955983
err = s.Conn().CloseWithError(42)
956984
require.NoError(t, err)
957985

@@ -960,16 +988,13 @@ func TestErrorCodes(t *testing.T) {
960988
Remote: false,
961989
})
962990

963-
remoteStream := <-remoteStreamQ
964-
defer remoteStream.Reset()
965-
966991
assertStreamErrors(remoteStream, &network.ConnError{
967992
ErrorCode: 42,
968993
Remote: true,
969994
})
970995
})
971996

972-
t.Run("StreamResetByConnCloseWithError", func(t *testing.T) {
997+
t.Run("NewStreamErrorByConnCloseWithError", func(t *testing.T) {
973998
if tc.Name == "WebTransport" || tc.Name == "WebRTC" {
974999
t.Skipf("skipping: %s, not implemented", tc.Name)
9751000
return

‎p2p/transport/quic/stream.go

+13-7
Original file line numberDiff line numberDiff line change
@@ -25,22 +25,28 @@ func parseStreamError(err error) error {
2525
}
2626
se := &quic.StreamError{}
2727
if errors.As(err, &se) {
28-
code := se.ErrorCode
29-
if code > math.MaxUint32 {
30-
// TODO(sukunrt): do we need this?
31-
code = reset
28+
var code network.StreamErrorCode
29+
if se.ErrorCode > math.MaxUint32 {
30+
code = network.StreamCodeOutOfRange
31+
} else {
32+
code = network.StreamErrorCode(se.ErrorCode)
3233
}
3334
err = &network.StreamError{
34-
ErrorCode: network.StreamErrorCode(code),
35+
ErrorCode: code,
3536
Remote: se.Remote,
3637
TransportError: se,
3738
}
3839
}
3940
ae := &quic.ApplicationError{}
4041
if errors.As(err, &ae) {
41-
code := ae.ErrorCode
42+
var code network.ConnErrorCode
43+
if ae.ErrorCode > math.MaxUint32 {
44+
code = network.ConnCodeOutOfRange
45+
} else {
46+
code = network.ConnErrorCode(ae.ErrorCode)
47+
}
4248
err = &network.ConnError{
43-
ErrorCode: network.ConnErrorCode(code),
49+
ErrorCode: code,
4450
Remote: ae.Remote,
4551
TransportError: ae,
4652
}

‎p2p/transport/webrtc/connection.go

+3-1
Original file line numberDiff line numberDiff line change
@@ -132,7 +132,9 @@ func (c *connection) Close() error {
132132
return nil
133133
}
134134

135-
func (c *connection) CloseWithError(errCode network.ConnErrorCode) error {
135+
// CloseWithError closes the connection ignoring the error code. As there's no way to signal
136+
// the remote peer on closing the underlying peerconnection, we ignore the error code.
137+
func (c *connection) CloseWithError(_ network.ConnErrorCode) error {
136138
return c.Close()
137139
}
138140

‎p2p/transport/webtransport/conn.go

+1-1
Original file line numberDiff line numberDiff line change
@@ -78,7 +78,7 @@ func (c *conn) Close() error {
7878
return err
7979
}
8080

81-
func (c *conn) CloseWithError(errCode network.ConnErrorCode) error {
81+
func (c *conn) CloseWithError(_ network.ConnErrorCode) error {
8282
return c.Close()
8383
}
8484

‎p2p/transport/webtransport/stream.go

+5
Original file line numberDiff line numberDiff line change
@@ -56,6 +56,11 @@ func (s *stream) Reset() error {
5656
return nil
5757
}
5858

59+
// ResetWithError resets the stream ignoring the error code. Error codes aren't
60+
// specified for WebTransport as the current implementation of WebTransport in
61+
// browsers(https://www.ietf.org/archive/id/draft-kinnear-webtransport-http2-02.html)
62+
// only supports 1 byte error codes. For more details, see
63+
// https://github.com/libp2p/specs/blob/4eca305185c7aef219e936bef76c48b1ab0a8b43/error-codes/README.md?plain=1#L84
5964
func (s *stream) ResetWithError(_ network.StreamErrorCode) error {
6065
s.Stream.CancelRead(reset)
6166
s.Stream.CancelWrite(reset)

0 commit comments

Comments
 (0)