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

plugin/transport: Prototype QUIC transport #176

Draft
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

coufalja
Copy link
Contributor

@coufalja coufalja commented Apr 30, 2021

Prototype UDP based transport #90

@coufalja coufalja changed the title transport - Prototype QUIC transport plugin/transport: Prototype QUIC transport Apr 30, 2021
@lni
Copy link
Owner

lni commented May 2, 2021

@coufalja thanks for the PR. a few issues and questions -

  1. in quicTransport.openStreamTo(), tls.Config.InsecureSkipVerify is always set to true. Any particular reason to do this?

  2. please add the copyright notice to each source file. the default copyright notice is provided below

// Copyright 2017-2021 Lei Ni (nilei81@gmail.com) and other contributors.
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
//     http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.
  1. please add some tests for this quic transport module, say in plugin/plugin_test.go.

  2. please add some comments to the factory.go to indicate that this transport module is experimental for now. such notice can be reviewed & removed later.

@coufalja
Copy link
Contributor Author

coufalja commented May 3, 2021

Thank you for a QUICk review, I am going to add tests soon. I've covered the other stuff already.

@lni
Copy link
Owner

lni commented May 9, 2021

@coufalja thanks for the update. A couple more quick questions -

  1. the CRC code has been removed, when TLS is not enabled, how data integrality is checked?

  2. for quicTransport.Start(), my understanding is that listener is like a server instance, listener.Accept() running inside a loop handles incoming connections from different clients, each of such connection is represented by a session instance s and s.AcceptStream() running inside its own loop is to keep getting byte streams sent by the client, but why there is another loop inside serveStream()? That is let's say if I have one client connected to the server and two pb.MessageBatch are sent to the server by the client on that connection, will the server side return two different stream instances and I get can get one pb.MessageBatch from each of those two stream instances or there will be only one stream instances with two pb.MessageBatch both can be read from it?

@coufalja
Copy link
Contributor Author

coufalja commented May 10, 2021

  1. the CRC code has been removed, when TLS is not enabled, how data integrality is checked?

The QUIC cannot be used without TLS, I have added an error return if you try to instantiate QUIC transport without TLS configured.

  1. for quicTransport.Start(), my understanding is that listener is like a server instance, listener.Accept() running inside a loop handles incoming connections from different clients, each of such connection is represented by a session instance s and s.AcceptStream() running inside its own loop is to keep getting byte streams sent by the client, but why there is another loop inside serveStream()? That is let's say if I have one client connected to the server and two pb.MessageBatch are sent to the server by the client on that connection, will the server side return two different stream instances and I get can get one pb.MessageBatch from each of those two stream instances or there will be only one stream instances with two pb.MessageBatch both can be read from it?

You are absolutely right 🤦 in it being wrong, I will fix it.

@lni
Copy link
Owner

lni commented May 10, 2021

@coufalja thanks for the input. I will update NodeHost to print out a warning message if NodeHost is created without TLS.

internal/transport/tests contains some keys & certificates that can be used in tests, please feel free to use them when adding tests to the quic plugin.

@lni
Copy link
Owner

lni commented May 23, 2021

@coufalja are you adding more tests? I can a see an added test covering the creation and shutdown of the quic transport module, some tests to check that the transport module actual works will be useful. thanks!

@vtolstov
Copy link

vtolstov commented Jan 5, 2024

any chance to get this completed and merged?

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.

None yet

3 participants