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

usagepool: Adds generic UsagePoolOf, deprecates UsagePool #5731

Closed
wants to merge 1 commit into from

Conversation

kkroo
Copy link
Contributor

@kkroo kkroo commented Aug 10, 2023

Adds much-needed type safety for UsagePool with a new type, UsagePoolOf.
UsagePool definition and public interfaces intact, with deprecation warning.
Updates usages within core modules, and adds specific value types to UsagePools, except for listenerPool, for which there is no interface common to quic.EarlyListener and the net package.

@kkroo kkroo force-pushed the generics_usagepool branch 2 times, most recently from 5d9ac36 to 7030d87 Compare August 11, 2023 00:45
@kkroo kkroo force-pushed the generics_usagepool branch from 7030d87 to d31392e Compare August 11, 2023 00:49
@mholt
Copy link
Member

mholt commented Aug 11, 2023

Ahhh, generics.

Thanks for the PR.

I suppose this makes sense. I'm not sure how much I love the [string, Destructor] pattern though, as it doesn't really give you much type-safety, does it? i.e. the Destructor can still be anything, you still need to type-assert, right?

(We're on feature freeze right now so this would have to wait until 2.9)

@mholt mholt added this to the 2.9.0 milestone Aug 11, 2023
@mholt mholt added the under review 🧐 Review is pending before merging label Aug 11, 2023
@kkroo
Copy link
Contributor Author

kkroo commented Aug 11, 2023

Golang is strongly typed.. anything but! Generics have some redeeming qualities

Ideally, listenerPool would be a [string, net.PacketConn] but the quic listener doesn't agree.

Destructor doesn't give much more type safety than now, but it cleans up the code a bit!

Feel free to commandeer this PR when you want to merge it

@kkroo
Copy link
Contributor Author

kkroo commented Aug 11, 2023

I wanted to introduce a DestructorOf[V] type but it would break the existing interface

@kkroo
Copy link
Contributor Author

kkroo commented Aug 12, 2023

Just read about 2.7 feature freeze. Wouldn't consider this a feature as much as code cleanup / hardening

@mholt
Copy link
Member

mholt commented Aug 12, 2023

I'm not against generics. I'm just careful to apply them at this stage, but getting more open to the idea as they get more mature. For example, I'd actually be happy to use min/max functions, for instance.

Type parameters, well... they're more complicated and I'm not sure what the actual gain here is.

Ideally, listenerPool would be a [string, net.PacketConn] but the quic listener doesn't agree.

listenerPool also has to store net.Listener and net.UnixConn values.

Wouldn't consider this a feature as much as code cleanup / hardening

I could possibly be convinced of this. However, this particular change is probably near the bottom of the priority list, and the implications of the change are still unknown to us, as generics are a very new feature of Go and this is a very crucial part of the code. So I'm wary is all. :)

This is an exported API as well; quite different from internal-only code. The stakes are a bit higher, so I want to take time to get it right.

I won't close this yet, but I just want time to consider it. Maybe have people try it out. I dunno. More discussion and experience.

@kkroo
Copy link
Contributor Author

kkroo commented Aug 12, 2023

There isn't a common interface to the listeners beyond Destructor and Closer which doesn't add much safety either.
listenerPool would need a refactor to introduce a more specific interface that is beyond the scope of this PR.
This PR doesn't change exposed interfaces since the original type is lexically and functionally identical to the [any, any] derived type signature. I've already gone through and tested with 3rd party plugins.
It does provide more safety for new development by error-prone type casts that are an eyesore.

@francislavoie francislavoie changed the title Adds generic UsagePoolOf, deprecates UsagePool usagepool: Adds generic UsagePoolOf, deprecates UsagePool Aug 14, 2023
@kkroo
Copy link
Contributor Author

kkroo commented Aug 15, 2023

looks like #5749 will make quic use a net.PacketConn and from quic-go/quic-go#3560 looks like it should be possible with unix listener as well

@mholt
Copy link
Member

mholt commented Sep 7, 2023

I'm not inclined to merge this at this stage due to the reasons I stated above, and as I think on it more, I don't find the benefits compelling, which as I understand it, boil down to:

error-prone type casts that are an eyesore.

While I suppose type assertions (not casts) are technically "error-prone", it's extremely difficult to do this in practice at two levels because:

  • Compilation will fail if you assert to the wrong type, in almost every practical use case
  • Running your code once, especially if you have any tests, will also make it obvious that a wrong type-assertion was made.

So I do appreciate the intent here to clean things up, but given the critical exported API, and the uncertainty of the implications and benefits at this stage, I'm going to close this for now. We can revisit this later though. :) Thank you so much for your time and effort!

@mholt mholt closed this Sep 7, 2023
@mholt mholt added declined 🚫 Not a fit for this project and removed under review 🧐 Review is pending before merging labels Sep 7, 2023
@mholt mholt removed this from the 2.9.0 milestone Sep 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
declined 🚫 Not a fit for this project
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants