Skip to content

Commit 7cfa0c4

Browse files
jasnelljasnell
committed
quic: address feedback and fix bugs
1 parent 0a59106 commit 7cfa0c4

File tree

4 files changed

+60
-25
lines changed

4 files changed

+60
-25
lines changed

‎src/quic/application.cc‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,4 @@
1+
#include "uv.h"
12
#if HAVE_OPENSSL && NODE_OPENSSL_HAS_QUIC
23

34
#include "application.h"
@@ -243,6 +244,7 @@ void Session::Application::SendPendingData() {
243244
}
244245
}
245246

247+
packet->Done(UV_ECANCELED);
246248
session_->last_error_ = QuicError::ForNgtcp2Error(nwrite);
247249
return session_->Close(Session::CloseMethod::SILENT);
248250
}
@@ -252,6 +254,7 @@ void Session::Application::SendPendingData() {
252254
// Since we are closing the session here, we don't worry about updating
253255
// the pkt tx time. The failed StreamCommit should have updated the
254256
// last_error_ appropriately.
257+
packet->Done(UV_ECANCELED);
255258
return session_->Close(Session::CloseMethod::SILENT);
256259
}
257260

‎src/quic/defs.h‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -9,9 +9,9 @@
99
namespace node {
1010
namespace quic {
1111

12-
#define NGTCP2_ERR(V) (V != 0)
13-
#define NGTCP2_OK(V) (V == 0)
1412
#define NGTCP2_SUCCESS 0
13+
#define NGTCP2_ERR(V) (V != NGTCP2_SUCCESS)
14+
#define NGTCP2_OK(V) (V == NGTCP2_SUCCESS)
1515

1616
template <typename Opt, std::string Opt::*member>
1717
bool SetOption(Environment* env,

‎src/quic/packet.cc‎

Lines changed: 37 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -199,11 +199,12 @@ int Packet::Send(uv_udp_t* handle, BaseObjectPtr<BaseObject> ref) {
199199
}
200200

201201
void Packet::Done(int status) {
202-
DCHECK_NOT_NULL(listener_);
203-
listener_->PacketDone(status);
202+
if (listener_ != nullptr) {
203+
listener_->PacketDone(status);
204+
}
205+
listener_ = nullptr;
204206
handle_.reset();
205207
data_.reset();
206-
listener_ = nullptr;
207208
Reset();
208209

209210
// As a performance optimization, we add this packet to a freelist
@@ -261,7 +262,10 @@ BaseObjectPtr<Packet> Packet::CreateRetryPacket(
261262
path_descriptor.dcid,
262263
vec.base,
263264
vec.len);
264-
if (nwrite <= 0) return BaseObjectPtr<Packet>();
265+
if (nwrite <= 0) {
266+
packet->Done(UV_ECANCELED);
267+
return BaseObjectPtr<Packet>();
268+
}
265269
packet->Truncate(static_cast<size_t>(nwrite));
266270
return packet;
267271
}
@@ -272,13 +276,16 @@ BaseObjectPtr<Packet> Packet::CreateConnectionClosePacket(
272276
const SocketAddress& destination,
273277
ngtcp2_conn* conn,
274278
const QuicError& error) {
275-
auto packet = Packet::Create(
279+
auto packet = Create(
276280
env, listener, destination, kDefaultMaxPacketLength, "connection close");
277281
ngtcp2_vec vec = *packet;
278282

279283
ssize_t nwrite = ngtcp2_conn_write_connection_close(
280284
conn, nullptr, nullptr, vec.base, vec.len, error, uv_hrtime());
281-
if (nwrite < 0) return BaseObjectPtr<Packet>();
285+
if (nwrite < 0) {
286+
packet->Done(UV_ECANCELED);
287+
return BaseObjectPtr<Packet>();
288+
}
282289
packet->Truncate(static_cast<size_t>(nwrite));
283290
return packet;
284291
}
@@ -288,11 +295,11 @@ BaseObjectPtr<Packet> Packet::CreateImmediateConnectionClosePacket(
288295
Listener* listener,
289296
const PathDescriptor& path_descriptor,
290297
const QuicError& reason) {
291-
auto packet = Packet::Create(env,
292-
listener,
293-
path_descriptor.remote_address,
294-
kDefaultMaxPacketLength,
295-
"immediate connection close (endpoint)");
298+
auto packet = Create(env,
299+
listener,
300+
path_descriptor.remote_address,
301+
kDefaultMaxPacketLength,
302+
"immediate connection close (endpoint)");
296303
ngtcp2_vec vec = *packet;
297304
ssize_t nwrite = ngtcp2_crypto_write_connection_close(
298305
vec.base,
@@ -305,7 +312,10 @@ BaseObjectPtr<Packet> Packet::CreateImmediateConnectionClosePacket(
305312
// there is one in the QuicError
306313
nullptr,
307314
0);
308-
if (nwrite <= 0) return BaseObjectPtr<Packet>();
315+
if (nwrite <= 0) {
316+
packet->Done(UV_ECANCELED);
317+
return BaseObjectPtr<Packet>();
318+
}
309319
packet->Truncate(static_cast<size_t>(nwrite));
310320
return packet;
311321
}
@@ -329,16 +339,17 @@ BaseObjectPtr<Packet> Packet::CreateStatelessResetPacket(
329339
uint8_t random[kRandlen];
330340
CHECK(crypto::CSPRNG(random, kRandlen).is_ok());
331341

332-
auto packet = Packet::Create(env,
333-
listener,
334-
path_descriptor.remote_address,
335-
kDefaultMaxPacketLength,
336-
"stateless reset");
342+
auto packet = Create(env,
343+
listener,
344+
path_descriptor.remote_address,
345+
kDefaultMaxPacketLength,
346+
"stateless reset");
337347
ngtcp2_vec vec = *packet;
338348

339349
ssize_t nwrite = ngtcp2_pkt_write_stateless_reset(
340350
vec.base, pktlen, token, random, kRandlen);
341351
if (nwrite <= static_cast<ssize_t>(kMinStatelessResetLen)) {
352+
packet->Done(UV_ECANCELED);
342353
return BaseObjectPtr<Packet>();
343354
}
344355

@@ -377,11 +388,11 @@ BaseObjectPtr<Packet> Packet::CreateVersionNegotiationPacket(
377388
size_t pktlen = path_descriptor.dcid.length() +
378389
path_descriptor.scid.length() + (sizeof(sv)) + 7;
379390

380-
auto packet = Packet::Create(env,
381-
listener,
382-
path_descriptor.remote_address,
383-
kDefaultMaxPacketLength,
384-
"version negotiation");
391+
auto packet = Create(env,
392+
listener,
393+
path_descriptor.remote_address,
394+
kDefaultMaxPacketLength,
395+
"version negotiation");
385396
ngtcp2_vec vec = *packet;
386397

387398
ssize_t nwrite =
@@ -394,7 +405,10 @@ BaseObjectPtr<Packet> Packet::CreateVersionNegotiationPacket(
394405
path_descriptor.scid.length(),
395406
sv,
396407
arraysize(sv));
397-
if (nwrite <= 0) return BaseObjectPtr<Packet>();
408+
if (nwrite <= 0) {
409+
packet->Done(UV_ECANCELED);
410+
return BaseObjectPtr<Packet>();
411+
}
398412
packet->Truncate(static_cast<size_t>(nwrite));
399413
return packet;
400414
}

‎src/quic/session.cc‎

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -467,6 +467,9 @@ Session::Session(BaseObjectPtr<Endpoint> endpoint,
467467
}
468468

469469
Session::~Session() {
470+
if (conn_closebuf_) {
471+
conn_closebuf_->Done(0);
472+
}
470473
if (qlog_stream_) {
471474
env()->SetImmediate(
472475
[ptr = std::move(qlog_stream_)](Environment*) { ptr->End(); });
@@ -721,13 +724,20 @@ bool Session::Receive(Store&& store,
721724
}
722725

723726
void Session::Send(BaseObjectPtr<Packet> packet) {
727+
// Sending a Packet is generally best effort. If we're not in a state
728+
// where we can send a packet, it's ok to drop it on the floor. The
729+
// packet loss mechanisms will cause the packet data to be resent later
730+
// if appropriate (and possible).
724731
DCHECK(!is_destroyed());
725732
DCHECK(!is_in_draining_period());
726733

727734
if (can_send_packets() && packet->length() > 0) {
728735
STAT_INCREMENT_N(Stats, bytes_sent, packet->length());
729736
endpoint_->Send(std::move(packet));
737+
return;
730738
}
739+
740+
packet->Done(packet->length() > 0 ? UV_ECANCELED : 0);
731741
}
732742

733743
void Session::Send(BaseObjectPtr<Packet> packet, const PathStorage& path) {
@@ -783,12 +793,14 @@ uint64_t Session::SendDatagram(Store&& data) {
783793
uv_hrtime());
784794

785795
if (nwrite < 0) {
796+
// Nothing was written to the packet.
786797
switch (nwrite) {
787798
case 0: {
788799
// We cannot send data because of congestion control or the data will
789800
// not fit. Since datagrams are best effort, we are going to abandon
790801
// the attempt and just return.
791802
CHECK_EQ(accepted, 0);
803+
packet->Done(UV_ECANCELED);
792804
return 0;
793805
}
794806
case NGTCP2_ERR_WRITE_MORE: {
@@ -798,20 +810,25 @@ uint64_t Session::SendDatagram(Store&& data) {
798810
case NGTCP2_ERR_INVALID_STATE: {
799811
// The remote endpoint does not want to accept datagrams. That's ok,
800812
// just return 0.
813+
packet->Done(UV_ECANCELED);
801814
return 0;
802815
}
803816
case NGTCP2_ERR_INVALID_ARGUMENT: {
804817
// The datagram is too large. That should have been caught above but
805818
// that's ok. We'll just abandon the attempt and return.
819+
packet->Done(UV_ECANCELED);
806820
return 0;
807821
}
808822
}
823+
packet->Done(UV_ECANCELED);
809824
last_error_ = QuicError::ForNgtcp2Error(nwrite);
810825
Close(CloseMethod::SILENT);
811826
return 0;
812827
}
813828

814829
// In this case, a complete packet was written and we need to send it along.
830+
// Note that this doesn't mean that the packet actually contains the datagram!
831+
// We'll check that next by checking the accepted value.
815832
packet->Truncate(nwrite);
816833
Send(std::move(packet));
817834
ngtcp2_conn_update_pkt_tx_time(*this, uv_hrtime());
@@ -1137,6 +1154,7 @@ void Session::SendConnectionClose() {
11371154
*this, &path, nullptr, vec.base, vec.len, last_error_, uv_hrtime());
11381155

11391156
if (UNLIKELY(nwrite < 0)) {
1157+
packet->Done(UV_ECANCELED);
11401158
last_error_ = QuicError::ForNgtcp2Error(NGTCP2_INTERNAL_ERROR);
11411159
Close(CloseMethod::SILENT);
11421160
} else {

0 commit comments

Comments
 (0)