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

replace AddHandler with Listen #79

Open
marten-seemann opened this issue Dec 17, 2021 · 2 comments
Open

replace AddHandler with Listen #79

marten-seemann opened this issue Dec 17, 2021 · 2 comments
Labels
effort/days Estimated to take multiple days, but less than a week exp/intermediate Prior experience is likely helpful kind/architecture Core architecture of project

Comments

@marten-seemann
Copy link
Contributor

@Stebalien suggested to replace the AddHandler (which gets passed a callback) with a Listen(... protocol.ID) function, returning a Listener, on which an Accept method could be called to accept streams.

This would be a more idiomatic approach, as it closely mirrors how net.Conns and streams are accepted. It would also move the go routine handling to the application, which is arguably the place where it belongs.

@marten-seemann marten-seemann added exp/novice Someone with a little familiarity can pick up kind/architecture Core architecture of project effort/days Estimated to take multiple days, but less than a week labels Dec 17, 2021
@Stebalien
Copy link
Member

Stebalien commented Dec 17, 2021 via email

@marten-seemann marten-seemann added exp/intermediate Prior experience is likely helpful and removed exp/novice Someone with a little familiarity can pick up labels Dec 19, 2021
@marten-seemann
Copy link
Contributor Author

marten-seemann commented Dec 19, 2021

This will be more involved than expected:
multistream is not only used for stream negotiation, but also to negotiate the security protocol (as long as we don't have Protocol Select) and the stream muxer. That's the reason go-multistream currently acts on io.ReadWriteClosers. It would be super nice to get rid of the type assertions and use generics:

type conn interface { // TODO: find a more descriptive name than conn
    io.ReadWriteCloser
    Reset() error
    Protocol() protocol.ID
}

type Listener[T conn] interface {
     io.Closer
     Accept() (T, error)
}

type MultistreamMuxer[T conn] struct {
     Listen(... protocol.ID) (Listener[T], error)
}

These types will be instantiated (or whatever the right generics-lingo term for this is) with a (wrapped) net.Conn, a network.SecureConn and a network.Stream.

We could then have an Accept function that works like this:

type listener {
    queue chan conn // buffered chan (of length 4 or 8)?
}

func (l *listener) addStream(c conn) { // called by the multistream muxer as soon as the protocol was negotiated
	select {
	case l.queue <- c:
	default:
		log.Warnf("dropping", "protocol", c.Protocol())
		c.Reset()
	}
}

func (l *listener) Accept() (network.Stream, error) {
	select {
	case <-l.closeChan:
		return nil, errors.New("listener closed")
	case str := <-l.queue:
		return str, nil
	}
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
effort/days Estimated to take multiple days, but less than a week exp/intermediate Prior experience is likely helpful kind/architecture Core architecture of project
Projects
None yet
Development

No branches or pull requests

2 participants