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

improvement: v0 enhancement and fix #11

Merged
merged 18 commits into from
Oct 28, 2023
Merged

Conversation

gaukas
Copy link
Contributor

@gaukas gaukas commented Oct 26, 2023

  • Rename v0plus to v0. v0plus is an internal code reference only.
  • Fix README.md.
  • Minor changes to interfaces.
  • Add test suites for Dialer/Listener/Relay in v0.

gaukas added 12 commits October 25, 2023 19:41
v0plus is an internal developer code reference and should keep internal.
Plus optimize the workflow for v0/relay
Since v0 is optionally imported, it is pointless to assign a special build tag to explicitly disable it.
ignoring err is unsafe
ignoring err is unsafe
ignoring err is unsafe
ignoring err is unsafe
ignoring err is unsafe
water benchmark bested std tcp... wut?
jmwample
jmwample previously approved these changes Oct 27, 2023
Copy link
Member

@jmwample jmwample left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This all looks like good improvement to me. The one thing that I want to double check on is that we are swapping from net interfaces to returning our own constructions because we need to access and use them with extra functionality, but they SHOULD still implement the net interfaces (i.e. net.Listener and net.Conn in listener.go and transport/v0/listener.go respectively). We should not change them to abandon those interfaces.
Other than that I would say this is ready to go 👍

listener.go Show resolved Hide resolved
@gaukas
Copy link
Contributor Author

gaukas commented Oct 27, 2023

As you can see in the type definition our Listener/Conn actually either embeds the net type (Conn embeds net.Conn) or resembles the net type (Listener is net.Listener with Accpet() modified to return Conn instead).

I am also a bit uncertain about the change on Accept(). Any suggestions? How about change it to embed net.Listener with AcceptWATER() which returns water.Conn? It will be more net-ish (like, net.TCPListener.AcceptTCP).

@jmwample
Copy link
Member

jmwample commented Oct 27, 2023

I guess the test is whether it can be cast / used as the interface externally by a caller, i.e.

var wListener net.Listener = water.Config{...}.Listen(...)
listenAndAccept(ln)

func listenAndAccept(ln net.Listener) error{
    for {
        c, _ := ln.Accept()
        handle(c)
    }
}

func handle(c net.Conn) error { ...

or transformed / accessed I guess, if this doesn't work we could do something like AsNetListener or AsNetConn that returns something implementing that interface. But that seems like extra steps for the same outcome

We can always make this the defacto for like internal usage. But for external APIs we want to try to present a consistent interface. I.E. is there somethings that we expect callers to be doing with a water.Conn that they can't do with a net.Conn or is it that we need to use it as a water.Conn?

@gaukas
Copy link
Contributor Author

gaukas commented Oct 27, 2023

You had a point. For it to pass the type assertion against net.Listener, the changes proposed above by me is pretty much needed. The type in the function prototype won't be inferred as interfaces it implements.

Just to clear things a bit: Currently water.Listener is essentially just a net.Listener but in the future we may found some extra functions useful to add. So I found it helpful to at least have a natural way of directly getting a water.Conn out of the water.Listener.

Typical users may keep using net.Listener interface and operate on net.Conn, but advanced users may prefer (*water.Listener).AcceptWATER (just like *net.TCPListener).AcceptTCP.)

Add `AcceptWATER()` in case it is useful to directly get `water.Conn` without unsafe type assertion.
Remove a few redundant defer calls.
Read from crypto/rand after each Write, which introduces "reasonable" latency as well.
Copy link
Member

@erikziyunchi erikziyunchi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me! Thanks for the benchmarking, it was a interesting finding!

@gaukas gaukas merged commit eb43f3b into master Oct 28, 2023
8 checks passed
@gaukas gaukas deleted the v0-structure-enhancement-fix branch November 28, 2023 22:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants