Copilot commented on code in PR #3508:
URL: https://github.com/apache/brpc/pull/3508#discussion_r3906110511
##########
test/brpc_rtmp_unittest.cpp:
##########
@@ -355,6 +355,14 @@ class PlayingDummyService : public brpc::RtmpService {
int64_t _sleep_ms;
};
+class RejectingRtmpService : public brpc::RtmpService {
+private:
+ brpc::RtmpServerStream* NewStream(
+ const brpc::RtmpConnectRequest&) override {
+ return nullptr;
+ }
+};
Review Comment:
Overriding `NewStream` under `private:` makes the customization point harder
to discover and can be surprising since RTMP service hooks are typically
`protected`/`public`. Consider changing the access specifier to `protected:`
(or `public:`) to align with the intent of an overridable service hook.
##########
test/brpc_rtmp_unittest.cpp:
##########
@@ -967,6 +975,43 @@ TEST(RtmpTest, abort_message_naming_own_chunk_stream) {
ASSERT_EQ(brpc::PARSE_OK, ctx.Feed(&buf2, sock.get()).error());
}
+TEST(RtmpTest, rejected_create_stream_with_response_write_failure) {
+ RejectingRtmpService rtmp_service;
+ brpc::Server server;
+ brpc::ServerOptions server_options;
+ server_options.rtmp_service = &rtmp_service;
+ ASSERT_EQ(0, server.Start(0, &server_options));
+
+ brpc::SocketId socket_id;
+ brpc::SocketOptions socket_options;
+ ASSERT_EQ(0, brpc::Socket::Create(socket_options, &socket_id));
+ brpc::SocketUniquePtr socket;
+ ASSERT_EQ(0, brpc::Socket::Address(socket_id, &socket));
+ ASSERT_EQ(0, socket->SetFailed());
+
+ brpc::policy::RtmpContext ctx(nullptr, &server);
+ brpc::policy::RtmpChunkStream chunk_stream(
+ &ctx, brpc::policy::RTMP_CONTROL_CHUNK_STREAM_ID);
+
+ std::string request;
+ google::protobuf::io::StringOutputStream zc_stream(&request);
+ brpc::AMFOutputStream ostream(&zc_stream);
+ brpc::WriteAMFNumber(1, &ostream);
+ brpc::AMFObject command;
+ brpc::WriteAMFObject(command, &ostream);
+ ASSERT_TRUE(ostream.good());
+
+ google::protobuf::io::ArrayInputStream input(
+ request.data(), request.size());
+ brpc::AMFInputStream istream(&input);
+ brpc::policy::RtmpMessageHeader message_header;
Review Comment:
`message_header` is default-declared without initialization; if
`RtmpMessageHeader` is a POD/aggregate without a constructor, its fields (e.g.,
`stream_id`) may be uninitialized and reading them inside `OnCreateStream` can
cause undefined behavior/flaky tests. Value-initialize it (e.g., `{}`) or
explicitly set the fields used by `OnCreateStream`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
