Introduce `netctx` package and `PacketConnCtx` #253

hasheddan · 2023-07-09T23:10:36Z

Description

Moves ConnCtx to the netctx package, which will be expanded to include
additional wrapped network interfaces.

Signed-off-by: Daniel Mangum [email protected]

Adds a PacketConnCtx that wraps net.PacketConn.

Signed-off-by: Daniel Mangum [email protected]

Reference issue

Enables pion/dtls#256

codecov · 2023-07-09T23:11:42Z

Codecov Report

Patch coverage: 76.14% and project coverage change: -0.56 ⚠️

Comparison is base (4b89bf8) 82.32% compared to head (f7257a2) 81.76%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #253      +/-   ##
==========================================
- Coverage   82.32%   81.76%   -0.56%     
==========================================
  Files          34       37       +3     
  Lines        3010     3203     +193     
==========================================
+ Hits         2478     2619     +141     
- Misses        421      460      +39     
- Partials      111      124      +13

Flag	Coverage Δ
go	`81.60% <76.14%> (-0.46%)`	⬇️
wasm	`74.26% <76.14%> (+0.10%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
connctx/connctx.go	`77.08% <ø> (ø)`
netctx/pipe.go	`0.00% <0.00%> (ø)`
netctx/packetconn.go	`76.59% <76.59%> (ø)`
netctx/conn.go	`77.08% <77.08%> (ø)`
test/stress.go	`76.19% <100.00%> (ø)`

... and 2 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

hasheddan · 2023-07-09T23:11:21Z

connctx/connctx.go

 // Package connctx wraps net.Conn using context.Context.
+// Deprecated: use netctx instead.
 package connctx


There are packages that currently depend on this, so we deprecate while introducing the same functionality in netctx.

stv0g · 2023-07-10T07:50:00Z

Hi @hasheddan,

can you briefly motivate why you renamed the existing package and deprecated the old name?

I am interested to learn about the differences between the two packages. Are there any API breaking changes in it?

hasheddan

@stv0g in adding PacketConnCtx, I had the option to introduce a new package or add it to the existing connctx package. The connctx package would be a bit of a misnomer for a PacketConnCtx, and introducing a new package such as packetconnctx would have the same duplicate naming that connctx currently has (i.e. connctx.ConnCtx and packetconnctx.PacketConnCtx). What we are really doing here is wrapping net primitives with context, so I instead introduced a package that matches upstream naming so that we have net.Conn / netctx.ConnCtx and net.PacketConn / netctx.PacketConnCtx. The API for ConnCtx remains the same, except for instead of New() the constructor is NewConnCtx(). The connctx package is kept around to not break existing consumers, but they will start getting linter warnings encouraging them to switch to netctx. We can choose to remove the connctx after a sufficient deprecation period if needed.

stv0g

Hi @hasheddan,

looks good to me :) I have a few smaller comments.
Ideally, we would also comment all the public methods.
I think here we can largely copy and adjust the respective single line comments from the net package.

We dont need to copy the full comments. But a single sentence per method would be nice.

stv0g · 2023-07-10T21:10:53Z

netctx/packetconnctx.go

+}
+
+// ReadFromWriterTo is a composite of ReadFromWriterTo.
+type ReadFromWriterTo interface {


Is this actually used somewhere?

It isn't -- it matches the ReadWriter in ConnCtx, but I don't think we need to include it until it is used 👍🏻

stv0g · 2023-07-10T21:11:34Z

netctx/packetconnctx.go

+}
+
+// PacketConnCtx is a wrapper of net.PacketConn using context.Context.
+type PacketConnCtx interface {


What do you think about removing the Ctx from the naming here? Isnt it implicitly given by the package name?

I'm a fan! Only kept it for consistency with the existing ConnCtx, but since we are moving it anyway I think we can use netctx.Conn and netctx.PacketConn 👍🏻

stv0g · 2023-07-10T21:13:02Z

netctx/packetconnctx.go

+}
+
+// NewPacketConn creates a new PacketConnCtx wrapping the given net.PacketConn.
+func NewPacketConn(pconn net.PacketConn) PacketConnCtx {


Also in reference to the previous commit. I would find it more harmonized if a NewXXX actually creates a XXX.
But, I would prefer to to simply rename the (Packet)?ConnCtx types to (Packet)?Conn.

stv0g · 2023-07-10T21:14:00Z

netctx/packetconnctx.go

+
+	select {
+	case <-p.closed:
+		return 0, nil, io.EOF


Isn't a net.ErrClosed more appropriate here?

That seems more reasonable to me 👍🏻

stv0g · 2023-07-10T21:14:33Z

netctx/connctx.go

+
+	select {
+	case <-c.closed:
+		return 0, io.EOF


Isn't a net.ErrClosed more appropriate here?

hasheddan

Thanks @stv0g! Updated based on your suggestions!

stv0g · 2023-07-11T09:45:03Z

Great :) You can merge it if you like

hasheddan

Thanks @stv0g! Just updated with the docstrings as well 👍🏻

Moves ConnCtx to the netctx package, which will be expanded to include additional wrapped network interfaces. Signed-off-by: Daniel Mangum <[email protected]>

Marks the connctx package as deprecated in favor of netctx. Signed-off-by: Daniel Mangum <[email protected]>

Updates to construct a new ConnCtx using NewConnCtx rather than New so that it does not conflict with other wrapped net interfaces. Signed-off-by: Daniel Mangum <[email protected]>

Adds a PacketConnCtx that wraps net.PacketConn. Signed-off-by: Daniel Mangum <[email protected]>

Adds unit tests for PacketConnCtx that match the existing unit tests for ConnCtx. Signed-off-by: Daniel Mangum <[email protected]>

Moves from using the now deprecated connctx to using netctx in stress tests. Signed-off-by: Daniel Mangum <[email protected]>

Refactors to use names that match the upstream net types. Signed-off-by: Daniel Mangum <[email protected]>

Updates to use net.ErrClosed when connection has been closed. Signed-off-by: Daniel Mangum <[email protected]>

Adds documentation for all public netctx methods. Signed-off-by: Daniel Mangum <[email protected]>

Adds Daniel Mangum to AUTHORS.txt. Signed-off-by: Daniel Mangum <[email protected]>

hasheddan commented Jul 9, 2023

View reviewed changes

hasheddan force-pushed the packetconnctx branch 2 times, most recently from f473f54 to 1fdb254 Compare July 9, 2023 23:22

hasheddan self-assigned this Jul 9, 2023

hasheddan commented Jul 10, 2023

View reviewed changes

hasheddan requested a review from stv0g July 10, 2023 19:01

stv0g requested changes Jul 10, 2023

View reviewed changes

hasheddan requested a review from stv0g July 11, 2023 01:50

hasheddan commented Jul 11, 2023

View reviewed changes

stv0g approved these changes Jul 11, 2023

View reviewed changes

hasheddan commented Jul 11, 2023

View reviewed changes

hasheddan force-pushed the packetconnctx branch from 52236ca to 742099f Compare July 11, 2023 10:39

hasheddan added 9 commits July 11, 2023 06:40

Move ConnCtx to the netctx package

5130177

Moves ConnCtx to the netctx package, which will be expanded to include additional wrapped network interfaces. Signed-off-by: Daniel Mangum <[email protected]>

Mark connctx package as deprecated

53cfcf1

Marks the connctx package as deprecated in favor of netctx. Signed-off-by: Daniel Mangum <[email protected]>

Use NewConnCtx instead of New

0daf3c1

Updates to construct a new ConnCtx using NewConnCtx rather than New so that it does not conflict with other wrapped net interfaces. Signed-off-by: Daniel Mangum <[email protected]>

Add context wrapped net.PacketConn

7db4567

Adds a PacketConnCtx that wraps net.PacketConn. Signed-off-by: Daniel Mangum <[email protected]>

Add unit tests for PacketConnCtx

9086c71

Adds unit tests for PacketConnCtx that match the existing unit tests for ConnCtx. Signed-off-by: Daniel Mangum <[email protected]>

Use netctx in stress tests

55f38ea

Moves from using the now deprecated connctx to using netctx in stress tests. Signed-off-by: Daniel Mangum <[email protected]>

Refactor ConnCtx/PacketConnCtx to Conn/PacketConn

900de14

Refactors to use names that match the upstream net types. Signed-off-by: Daniel Mangum <[email protected]>

Use net.ErrClosed instead of io.EOF

f77fa01

Updates to use net.ErrClosed when connection has been closed. Signed-off-by: Daniel Mangum <[email protected]>

Document all public netctx methods

ae28900

Adds documentation for all public netctx methods. Signed-off-by: Daniel Mangum <[email protected]>

hasheddan force-pushed the packetconnctx branch from 742099f to ae28900 Compare July 11, 2023 10:40

Add Daniel Mangum to AUTHORS.txt

f7257a2

Adds Daniel Mangum to AUTHORS.txt. Signed-off-by: Daniel Mangum <[email protected]>

hasheddan merged commit a789100 into pion:master Jul 11, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce `netctx` package and `PacketConnCtx` #253

Introduce `netctx` package and `PacketConnCtx` #253

hasheddan commented Jul 9, 2023

codecov bot commented Jul 9, 2023 •

edited

Loading

hasheddan Jul 9, 2023

stv0g commented Jul 10, 2023

hasheddan left a comment

stv0g left a comment

stv0g Jul 10, 2023

hasheddan Jul 11, 2023

stv0g Jul 10, 2023

hasheddan Jul 10, 2023

stv0g Jul 10, 2023

stv0g Jul 10, 2023

hasheddan Jul 11, 2023

stv0g Jul 10, 2023

hasheddan left a comment

stv0g commented Jul 11, 2023

hasheddan left a comment

Introduce netctx package and PacketConnCtx #253

Introduce netctx package and PacketConnCtx #253

Conversation

hasheddan commented Jul 9, 2023

Description

Reference issue

codecov bot commented Jul 9, 2023 • edited Loading

Codecov Report

Choose a reason for hiding this comment

stv0g commented Jul 10, 2023

hasheddan left a comment

Choose a reason for hiding this comment

stv0g left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hasheddan left a comment

Choose a reason for hiding this comment

stv0g commented Jul 11, 2023

hasheddan left a comment

Choose a reason for hiding this comment

Introduce `netctx` package and `PacketConnCtx` #253

Introduce `netctx` package and `PacketConnCtx` #253

codecov bot commented Jul 9, 2023 •

edited

Loading