Copilot commented on code in PR #3508:
URL: https://github.com/apache/brpc/pull/3508#discussion_r3892885420
##########
src/brpc/policy/rtmp_protocol.cpp:
##########
@@ -2714,9 +2714,11 @@ bool RtmpChunkStream::OnCreateStream(const
RtmpMessageHeader& mh,
PLOG(WARNING) << socket->remote_side() << '[' << mh.stream_id
<< "] Fail to respond createStream";
// End the stream at server-side.
- const bthread_id_t id = stream->_onfail_id;
- if (id != INVALID_BTHREAD_ID) {
- bthread_id_error(id, 0);
+ if (stream != nullptr) {
+ const bthread_id_t id = stream->_onfail_id;
+ if (id != INVALID_BTHREAD_ID) {
+ bthread_id_error(id, 0);
+ }
}
return false;
Review Comment:
The null-stream guard fixes a crash in an error path (service rejects
createStream by returning null, then the server fails to write the rejection
response). There’s currently no unit test exercising this rejected-createStream
+ response-write-failure path, so regressions would be easy to reintroduce.
Consider adding a test in test/brpc_rtmp_unittest.cpp with a custom
RtmpService::NewStream() that returns nullptr, then have the client close the
connection immediately after issuing createStream so the server’s response
write fails; the test should assert the server stays alive and no crash occurs.
--
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]
