1
0
mirror of https://github.com/facebookincubator/mvfst.git synced 2025-07-29 03:41:11 +03:00

Elevate exceptions out of writeStreamFrameHeader

Summary: Return an error and throw in callers instead.

Reviewed By: sharmafb

Differential Revision: D70011330

fbshipit-source-id: 9dc16d0f67ac13c58c3d89132d3bdc0c99e0cdd9
This commit is contained in:
Konstantin Tsoy
2025-02-25 09:02:01 -08:00
committed by Facebook GitHub Bot
parent 71b4e1c613
commit 7ba66a2d77
10 changed files with 118 additions and 42 deletions

View File

@ -358,7 +358,7 @@ bool StreamFrameScheduler::writeStreamLossBuffers(
buffer != stream.lossBuffer.cend();
++buffer) {
auto bufferLen = buffer->data.chainLength();
auto dataLen = writeStreamFrameHeader(
auto res = writeStreamFrameHeader(
builder,
stream.id,
buffer->offset,
@ -367,6 +367,11 @@ bool StreamFrameScheduler::writeStreamLossBuffers(
buffer->eof,
none /* skipLenHint */,
stream.groupId);
if (res.hasError()) {
throw QuicInternalException(
res.error().message, *res.error().code.asLocalErrorCode());
}
auto dataLen = *res;
if (dataLen) {
wroteStreamFrame = true;
writeStreamFrameData(builder, buffer->data, *dataLen);
@ -528,7 +533,7 @@ bool StreamFrameScheduler::writeStreamFrame(
bool canWriteFin = stream.finalWriteOffset.has_value() &&
bufferLen <= flowControlLen && stream.writeBufMeta.offset == 0;
auto writeOffset = stream.currentWriteOffset;
auto dataLen = writeStreamFrameHeader(
auto res = writeStreamFrameHeader(
builder,
stream.id,
writeOffset,
@ -537,6 +542,11 @@ bool StreamFrameScheduler::writeStreamFrame(
canWriteFin,
none /* skipLenHint */,
stream.groupId);
if (res.hasError()) {
throw QuicInternalException(
res.error().message, *res.error().code.asLocalErrorCode());
}
auto dataLen = *res;
if (!dataLen) {
return false;
}

View File

@ -94,7 +94,6 @@ mvfst_cpp_library(
"-fstrict-aliasing",
],
deps = [
"//folly:string",
"//quic:exception",
],
exported_deps = [
@ -163,7 +162,6 @@ mvfst_cpp_library(
],
deps = [
":decode",
"//quic:exception",
],
exported_deps = [
":packet_number",

View File

@ -91,7 +91,7 @@ Optional<ClonedPacketIdentifier> PacketRebuilder::rebuildFromPacket(
if (stream && retransmittable(*stream)) {
auto streamData = cloneRetransmissionBuffer(streamFrame, stream);
auto bufferLen = streamData ? streamData->chainLength() : 0;
auto dataLen = writeStreamFrameHeader(
auto res = writeStreamFrameHeader(
builder_,
streamFrame.streamId,
streamFrame.offset,
@ -103,6 +103,11 @@ Optional<ClonedPacketIdentifier> PacketRebuilder::rebuildFromPacket(
// frame last we need to end the stream frame in that case.
lastFrame && bufferLen && !hasAckFrame,
streamFrame.streamGroupId);
if (res.hasError()) {
throw QuicInternalException(
res.error().message, *res.error().code.asLocalErrorCode());
}
auto dataLen = *res;
bool ret = dataLen.has_value() && *dataLen == streamFrame.len;
if (ret) {
// Writing 0 byte for stream data is legit if the stream frame has

View File

@ -28,7 +28,7 @@ bool packetSpaceCheck(uint64_t limit, size_t require) {
namespace quic {
Optional<uint64_t> writeStreamFrameHeader(
folly::Expected<Optional<uint64_t>, QuicError> writeStreamFrameHeader(
PacketBuilderInterface& builder,
StreamId id,
uint64_t offset,
@ -42,9 +42,9 @@ Optional<uint64_t> writeStreamFrameHeader(
return none;
}
if (writeBufferLen == 0 && !fin) {
throw QuicInternalException(
"No data or fin supplied when writing stream.",
LocalErrorCode::INTERNAL_ERROR);
return folly::makeUnexpected(QuicError(
LocalErrorCode::INTERNAL_ERROR,
"No data or fin supplied when writing stream."));
}
StreamTypeField::Builder streamTypeBuilder;
if (streamGroupId) {

View File

@ -49,7 +49,7 @@ size_t writeFrame(QuicWriteFrame&& frame, PacketBuilderInterface& builder);
* to decide it. When skipLenHint is true, the field is skipped. When it's
* false, it will be encoded into the header.
*/
Optional<uint64_t> writeStreamFrameHeader(
folly::Expected<Optional<uint64_t>, QuicError> writeStreamFrameHeader(
PacketBuilderInterface& builder,
StreamId id,
uint64_t offset,

View File

@ -496,8 +496,10 @@ TEST_F(QuicWriteCodecTest, WriteStreamFrameToEmptyPacket) {
StreamId streamId = 1;
uint64_t offset = 0;
bool fin = false;
auto dataLen = writeStreamFrameHeader(
auto res = writeStreamFrameHeader(
pktBuilder, streamId, offset, 10, 10, fin, none /* skipLenHint */);
ASSERT_TRUE(res.hasValue());
auto dataLen = *res;
ASSERT_TRUE(dataLen);
ASSERT_EQ(*dataLen, 10);
writeStreamFrameData(pktBuilder, inputBuf->clone(), 10);
@ -542,8 +544,10 @@ TEST_F(QuicWriteCodecTest, WriteStreamFrameToPartialPacket) {
// 4 bytes offset
// 1 byte for length
// => 8 bytes of header
auto dataLen = writeStreamFrameHeader(
auto res = writeStreamFrameHeader(
pktBuilder, streamId, offset, 20, 20, fin, none /* skipLenHint */);
ASSERT_TRUE(res.hasValue());
auto dataLen = *res;
ASSERT_TRUE(dataLen);
EXPECT_EQ(*dataLen, 20);
writeStreamFrameData(pktBuilder, inputBuf->clone(), 20);
@ -591,8 +595,10 @@ TEST_F(QuicWriteCodecTest, WriteTwoStreamFrames) {
uint64_t offset1 = 65535;
bool fin1 = false;
auto inputBuf = buildRandomInputData(30);
auto dataLen = writeStreamFrameHeader(
auto res = writeStreamFrameHeader(
pktBuilder, streamId1, offset1, 30, 30, fin1, none /* skipLenHint */);
ASSERT_TRUE(res.hasValue());
auto dataLen = *res;
ASSERT_TRUE(dataLen);
ASSERT_EQ(*dataLen, 30);
writeStreamFrameData(pktBuilder, inputBuf->clone(), 30);
@ -612,7 +618,7 @@ TEST_F(QuicWriteCodecTest, WriteTwoStreamFrames) {
// 2 bytes for stream
// 4 bytes for offset
// => 7 bytes
dataLen = writeStreamFrameHeader(
res = writeStreamFrameHeader(
pktBuilder,
streamId2,
offset2,
@ -620,6 +626,8 @@ TEST_F(QuicWriteCodecTest, WriteTwoStreamFrames) {
remainingSpace,
fin2,
none /* skipLenHint */);
ASSERT_TRUE(res.hasValue());
dataLen = *res;
ASSERT_TRUE(dataLen);
ASSERT_EQ(*dataLen, remainingSpace - 7);
writeStreamFrameData(pktBuilder, inputBuf2->clone(), remainingSpace - 7);
@ -688,8 +696,10 @@ TEST_F(QuicWriteCodecTest, WriteStreamFramePartialData) {
// 2 bytes for stream id
// 4 bytes for offset
// => 7 bytes for header
auto dataLen = writeStreamFrameHeader(
auto res = writeStreamFrameHeader(
pktBuilder, streamId, offset, 50, 50, fin, none /* skipLenHint */);
ASSERT_TRUE(res.hasValue());
auto dataLen = *res;
ASSERT_TRUE(dataLen);
ASSERT_EQ(*dataLen, 33);
writeStreamFrameData(pktBuilder, inputBuf->clone(), 33);
@ -727,8 +737,10 @@ TEST_F(QuicWriteCodecTest, WriteStreamFrameTooSmallForStreamHeader) {
StreamId streamId = 1;
uint64_t offset = 65535;
bool fin = false;
auto dataLen = writeStreamFrameHeader(
auto res = writeStreamFrameHeader(
pktBuilder, streamId, offset, 1, 1, fin, none /* skipLenHint */);
EXPECT_TRUE(res.hasValue());
auto dataLen = *res;
EXPECT_FALSE(dataLen);
EXPECT_EQ(1, pktBuilder.remainingSpaceInPkt());
}
@ -745,8 +757,10 @@ TEST_F(QuicWriteCodecTest, WriteStreamNoSpaceForData) {
// 1 byte for stream id
// 1 byte for offset
// => 3 bytes
auto dataLen = writeStreamFrameHeader(
auto res = writeStreamFrameHeader(
pktBuilder, streamId, offset, 10, 10, fin, none /* skipLenHint */);
ASSERT_TRUE(res.hasValue());
auto dataLen = *res;
EXPECT_FALSE(dataLen.has_value());
EXPECT_EQ(pktBuilder.remainingSpaceInPkt(), 3);
}
@ -767,8 +781,10 @@ TEST_F(QuicWriteCodecTest, WriteStreamSpaceForOneByte) {
// 1 byte for stream id
// 1 byte for offset
// => 3 bytes
auto dataLen = writeStreamFrameHeader(
auto res = writeStreamFrameHeader(
pktBuilder, streamId, offset, 100, 100, fin, none /* skipLenHint */);
ASSERT_TRUE(res.hasValue());
auto dataLen = *res;
ASSERT_TRUE(dataLen);
ASSERT_EQ(*dataLen, 1);
writeStreamFrameData(pktBuilder, inputBuf->clone(), 1);
@ -813,8 +829,10 @@ TEST_F(QuicWriteCodecTest, WriteFinToEmptyPacket) {
StreamId streamId = 1;
uint64_t offset = 0;
bool fin = true;
auto dataLen = writeStreamFrameHeader(
auto res = writeStreamFrameHeader(
pktBuilder, streamId, offset, 10, 10, fin, none /* skipLenHint */);
ASSERT_TRUE(res.hasValue());
auto dataLen = *res;
ASSERT_TRUE(dataLen);
ASSERT_EQ(*dataLen, 10);
writeStreamFrameData(pktBuilder, inputBuf->clone(), 10);
@ -864,7 +882,7 @@ TEST_F(QuicWriteCodecTest, TestWriteIncompleteDataAndFin) {
StreamId streamId = 1;
uint64_t offset = 0;
bool fin = true;
auto dataLen = writeStreamFrameHeader(
auto res = writeStreamFrameHeader(
pktBuilder,
streamId,
offset,
@ -872,6 +890,8 @@ TEST_F(QuicWriteCodecTest, TestWriteIncompleteDataAndFin) {
inDataSize,
fin,
none /* skipLenHint */);
ASSERT_TRUE(res.hasValue());
auto dataLen = *res;
ASSERT_TRUE(dataLen);
EXPECT_LT(*dataLen, inDataSize);
}
@ -887,8 +907,10 @@ TEST_F(QuicWriteCodecTest, TestWriteNoDataAndFin) {
uint64_t offset = 0;
bool fin = true;
Buf empty;
auto dataLen = writeStreamFrameHeader(
auto res = writeStreamFrameHeader(
pktBuilder, streamId, offset, 0, 0, fin, none /* skipLenHint */);
ASSERT_TRUE(res.hasValue());
auto dataLen = *res;
ASSERT_TRUE(dataLen);
EXPECT_EQ(*dataLen, 0);
}
@ -900,10 +922,14 @@ TEST_F(QuicWriteCodecTest, TestWriteNoDataAndNoFin) {
uint64_t offset = 0;
bool fin = false;
Buf empty;
EXPECT_THROW(
writeStreamFrameHeader(
pktBuilder, streamId, offset, 0, 0, fin, none /* skipLenHint */),
QuicInternalException);
auto res = writeStreamFrameHeader(
pktBuilder, streamId, offset, 0, 0, fin, none /* skipLenHint */);
EXPECT_TRUE(res.hasError());
EXPECT_EQ(
res.error(),
QuicError(
LocalErrorCode::INTERNAL_ERROR,
"No data or fin supplied when writing stream."));
}
TEST_F(QuicWriteCodecTest, PacketOnlyHasSpaceForStreamHeader) {
@ -919,8 +945,10 @@ TEST_F(QuicWriteCodecTest, PacketOnlyHasSpaceForStreamHeader) {
StreamId streamId = 1;
uint64_t offset = 0;
bool fin = true;
auto dataLen = writeStreamFrameHeader(
auto res = writeStreamFrameHeader(
pktBuilder, streamId, offset, 20, 20, fin, none /* skipLenHint */);
ASSERT_TRUE(res.hasValue());
auto dataLen = *res;
EXPECT_FALSE(dataLen.has_value());
EXPECT_EQ(pktBuilder.remainingSpaceInPkt(), 2);
}
@ -936,8 +964,10 @@ TEST_F(QuicWriteCodecTest, PacketOnlyHasSpaceForStreamHeaderWithFin) {
StreamId streamId = 1;
uint64_t offset = 0;
bool fin = true;
auto dataLen = writeStreamFrameHeader(
auto res = writeStreamFrameHeader(
pktBuilder, streamId, offset, 0, 0, fin, none /* skipLenHint */);
ASSERT_TRUE(res.hasValue());
auto dataLen = *res;
ASSERT_TRUE(dataLen.has_value());
EXPECT_EQ(*dataLen, 0);
EXPECT_EQ(pktBuilder.remainingSpaceInPkt(), 0);
@ -954,8 +984,10 @@ TEST_F(QuicWriteCodecTest, PacketNotEnoughSpaceForStreamHeaderWithFin) {
StreamId streamId = 1;
uint64_t offset = 0;
bool fin = true;
auto dataLen = writeStreamFrameHeader(
auto res = writeStreamFrameHeader(
pktBuilder, streamId, offset, 0, 0, fin, none /* skipLenHint */);
EXPECT_TRUE(res.hasValue());
auto dataLen = *res;
ASSERT_FALSE(dataLen.has_value());
EXPECT_EQ(pktBuilder.remainingSpaceInPkt(), 2);
}
@ -980,8 +1012,10 @@ TEST_F(QuicWriteCodecTest, WriteStreamFrameHeadeSkipLen) {
StreamId streamId = 0;
uint64_t offset = 10;
bool fin = false;
auto dataLen = writeStreamFrameHeader(
auto res = writeStreamFrameHeader(
pktBuilder, streamId, offset, 1200 * 2, 1200 * 2, fin, none);
EXPECT_TRUE(res.hasValue());
auto dataLen = *res;
EXPECT_LT(*dataLen, 1200);
}
@ -1005,8 +1039,10 @@ TEST_F(QuicWriteCodecTest, WriteStreamFrameHeadeNotSkipLen) {
StreamId streamId = 0;
uint64_t offset = 10;
bool fin = false;
auto dataLen =
auto res =
writeStreamFrameHeader(pktBuilder, streamId, offset, 200, 200, fin, none);
EXPECT_TRUE(res.hasValue());
auto dataLen = *res;
EXPECT_EQ(*dataLen, 200);
}
@ -1030,8 +1066,10 @@ TEST_F(QuicWriteCodecTest, WriteStreamFrameHeadeLengthHintTrue) {
StreamId streamId = 0;
uint64_t offset = 10;
bool fin = false;
auto dataLen =
auto res =
writeStreamFrameHeader(pktBuilder, streamId, offset, 200, 200, fin, true);
EXPECT_TRUE(res.hasValue());
auto dataLen = *res;
EXPECT_EQ(*dataLen, 200);
}
@ -1055,8 +1093,10 @@ TEST_F(QuicWriteCodecTest, WriteStreamFrameHeadeLengthHintFalse) {
StreamId streamId = 0;
uint64_t offset = 10;
bool fin = false;
auto dataLen = writeStreamFrameHeader(
auto res = writeStreamFrameHeader(
pktBuilder, streamId, offset, 1200 * 2, 1200 * 2, fin, false);
EXPECT_TRUE(res.hasValue());
auto dataLen = *res;
EXPECT_LT(*dataLen, 1200);
}
@ -2587,7 +2627,7 @@ TEST_F(QuicWriteCodecTest, WriteStreamFrameWithGroup) {
uint64_t offset = 0;
bool fin = true;
auto dataLen = writeStreamFrameHeader(
auto res = writeStreamFrameHeader(
pktBuilder,
streamId,
offset,
@ -2596,6 +2636,8 @@ TEST_F(QuicWriteCodecTest, WriteStreamFrameWithGroup) {
fin,
none /* skipLenHint */,
groupId);
ASSERT_TRUE(res.hasValue());
auto dataLen = *res;
ASSERT_TRUE(dataLen);
ASSERT_EQ(*dataLen, 50);
writeStreamFrameData(pktBuilder, inputBuf->clone(), 50);

View File

@ -305,7 +305,7 @@ RegularQuicPacketBuilder::Packet createStreamPacket(
}
builder->encodePacketHeader();
builder->accountForCipherOverhead(cipherOverhead);
auto dataLen = *writeStreamFrameHeader(
auto res = *writeStreamFrameHeader(
*builder,
streamId,
offset,
@ -313,6 +313,8 @@ RegularQuicPacketBuilder::Packet createStreamPacket(
data.computeChainDataLength(),
eof,
none /* skipLenHint */);
CHECK(res.hasValue()) << "failed to write stream frame header";
auto dataLen = *res;
auto dataBuf = data.clone();
writeStreamFrameData(
*builder,

View File

@ -42,7 +42,7 @@ bool PacketGroupWriter::writeSingleQuicPacket(
builder.accountForCipherOverhead(aead.getCipherOverhead());
// frontend has already limited the length to flow control, thus
// flowControlLen == length
auto dataLen = writeStreamFrameHeader(
auto res = writeStreamFrameHeader(
builder,
streamId,
offset,
@ -52,6 +52,11 @@ bool PacketGroupWriter::writeSingleQuicPacket(
true /* skip length field in stream header */,
std::nullopt, /* stream group id */
false /* don't append frame to builder */);
if (res.hasError()) {
throw QuicInternalException(
res.error().message, *res.error().code.asLocalErrorCode());
}
auto dataLen = *res;
ChainedByteRangeHead chainedByteRangeHead(buf);
writeStreamFrameData(builder, chainedByteRangeHead, *dataLen);
auto packet = std::move(builder).buildPacket();

View File

@ -1461,7 +1461,7 @@ auto createInitialStream(
0 /* largestAcked */);
builder.encodePacketHeader();
auto streamData = data.clone();
auto dataLen = writeStreamFrameHeader(
auto res = writeStreamFrameHeader(
builder,
streamId,
0,
@ -1469,6 +1469,8 @@ auto createInitialStream(
streamData->computeChainDataLength(),
true,
none /* skipLenHint */);
EXPECT_TRUE(res.hasValue());
auto dataLen = *res;
EXPECT_TRUE(dataLen);
writeStreamFrameData(builder, std::move(streamData), *dataLen);
return packetToBuf(std::move(builder).buildPacket());

View File

@ -87,7 +87,7 @@ TEST_F(QuicServerTransportTest, TestReadMultipleStreams) {
auto buf1 = IOBuf::copyBuffer("Aloha");
auto buf2 = IOBuf::copyBuffer("Hello");
auto dataLen = writeStreamFrameHeader(
auto res = writeStreamFrameHeader(
builder,
0x08,
0,
@ -95,11 +95,13 @@ TEST_F(QuicServerTransportTest, TestReadMultipleStreams) {
buf1->computeChainDataLength(),
true,
none /* skipLenHint */);
ASSERT_TRUE(res.hasValue());
auto dataLen = *res;
ASSERT_TRUE(dataLen);
ASSERT_EQ(*dataLen, buf1->computeChainDataLength());
writeStreamFrameData(builder, buf1->clone(), buf1->computeChainDataLength());
dataLen = writeStreamFrameHeader(
res = writeStreamFrameHeader(
builder,
0x0C,
0,
@ -107,6 +109,8 @@ TEST_F(QuicServerTransportTest, TestReadMultipleStreams) {
buf1->computeChainDataLength(),
true,
none /* skipLenHint */);
ASSERT_TRUE(res.hasValue());
dataLen = *res;
ASSERT_TRUE(dataLen);
ASSERT_EQ(*dataLen, buf1->computeChainDataLength());
writeStreamFrameData(builder, buf2->clone(), buf2->computeChainDataLength());
@ -1058,7 +1062,7 @@ TEST_F(QuicServerTransportTest, RecvStopSendingFrameAfterHalfCloseRemote) {
StopSendingFrame stopSendingFrame(
streamId, GenericApplicationErrorCode::UNKNOWN);
ASSERT_TRUE(builder.canBuildPacket());
auto dataLen = writeStreamFrameHeader(
auto res = writeStreamFrameHeader(
builder,
0x00,
stream->currentReadOffset,
@ -1066,6 +1070,8 @@ TEST_F(QuicServerTransportTest, RecvStopSendingFrameAfterHalfCloseRemote) {
10,
true,
none /* skipLenHint */);
ASSERT_TRUE(res.hasValue());
auto dataLen = *res;
ASSERT_TRUE(dataLen.has_value());
ASSERT_EQ(*dataLen, 0);
writeFrame(QuicSimpleFrame(stopSendingFrame), builder);
@ -3789,7 +3795,7 @@ TEST_F(
// add some data
auto data = IOBuf::copyBuffer("hello!");
auto dataLen = *writeStreamFrameHeader(
auto res = *writeStreamFrameHeader(
builder,
/*id=*/4,
/*offset=*/0,
@ -3797,6 +3803,8 @@ TEST_F(
data->computeChainDataLength(),
/*fin=*/true,
/*skipLenHint=*/none);
ASSERT_TRUE(res.hasValue());
auto dataLen = *res;
writeStreamFrameData(
builder,
data->clone(),
@ -3830,7 +3838,7 @@ TEST_F(
// add some data
auto data = IOBuf::copyBuffer("hello!");
auto dataLen = *writeStreamFrameHeader(
auto res = *writeStreamFrameHeader(
builder,
/*id=*/4,
/*offset=*/0,
@ -3838,6 +3846,8 @@ TEST_F(
data->computeChainDataLength(),
/*eof=*/true,
/*skipLenHint=*/none);
ASSERT_TRUE(res.hasValue());
auto dataLen = *res;
writeStreamFrameData(
builder,
data->clone(),
@ -3871,7 +3881,7 @@ TEST_F(
// add some data
auto data = IOBuf::copyBuffer("hello!");
auto dataLen = *writeStreamFrameHeader(
auto res = *writeStreamFrameHeader(
builder,
/*id=*/4,
/*offset=*/0,
@ -3879,6 +3889,8 @@ TEST_F(
data->computeChainDataLength(),
/*eof=*/true,
/*skipLenHint=*/none);
ASSERT_TRUE(res.hasValue());
auto dataLen = *res;
writeStreamFrameData(
builder,
data->clone(),