Copilot commented on code in PR #3525:
URL: https://github.com/apache/brpc/pull/3525#discussion_r3951530705
##########
test/brpc_server_unittest.cpp:
##########
@@ -1719,6 +1721,158 @@ TEST_F(ServerTest,
builtin_services_are_gated_by_internal_port) {
ASSERT_EQ(0, server.Join());
}
+// Call the same ordinary service the way a browser would.
+void CallEchoByHttp(const butil::EndPoint& ep, brpc::Controller* cntl) {
+ brpc::ChannelOptions copt;
+ copt.protocol = brpc::PROTOCOL_HTTP;
+ copt.max_retry = 0;
+ brpc::Channel chan;
+ ASSERT_EQ(0, chan.Init(ep, &copt));
+ test::EchoRequest req;
+ test::EchoResponse res;
+ req.set_message(EXP_REQUEST);
+ cntl->http_request().uri() = "/EchoService/Echo";
+ cntl->http_request().set_method(brpc::HTTP_METHOD_POST);
+ cntl->http_request().set_content_type("application/json");
+ chan.CallMethod(nullptr, cntl, &req, &res, nullptr);
+}
+
+// Returns a port nothing is listening on, or -1. `ServerOptions.internal_port`
+// has to be an explicit number, Server::Start() rejects 0 because it stands
+// for an ephemeral port, so ask the system for a free one rather than hardcode
+// a port that another test may be listening on.
+int PickUnusedPort() {
+ butil::fd_guard sockfd(butil::tcp_listen(butil::EndPoint(butil::IP_ANY,
0)));
+ if (sockfd < 0) {
+ return -1;
+ }
+ butil::EndPoint point;
+ if (butil::get_local_side(sockfd, &point) != 0) {
+ return -1;
+ }
+ return point.port;
+}
+
+// Starts `server` on an ephemeral port and fills `options->internal_port` with
+// another one. Both are released before Start() binds them and something else
+// may take one in between, hence the retries. Returns 0 on success.
+int StartWithInternalPort(brpc::Server* server, brpc::ServerOptions* options) {
+ for (int i = 0; i < 10; ++i) {
+ int internal_port = PickUnusedPort();
+ if (internal_port < 0) {
+ continue;
+ }
+ options->internal_port = internal_port;
+ if (0 == server->Start("127.0.0.1:0", options)) {
+ return 0;
+ }
+ }
+ return -1;
+}
Review Comment:
These new test helper functions have external linkage (non-`static` at
namespace scope). In large test binaries this can cause avoidable symbol/ODR
collisions. Prefer giving them internal linkage (e.g., `static` functions or
wrapping them in an anonymous namespace) since they’re only used within this TU.
##########
src/brpc/details/server_private_accessor.h:
##########
@@ -116,17 +129,42 @@ class ServerPrivateAccessor {
// this beforehand, or make sure the listing is hidden in security mode.
inline bool RejectBuiltinAccess(Controller* cntl, const Server& server,
const Server::MethodProperty* mp) {
- if (!cntl->is_security_mode() ||
- (!mp->is_builtin_service && !mp->params.is_tabbed)) {
+ if (!cntl->is_security_mode() || !IsBuiltinOrTabbed(mp)) {
+ return false;
+ }
+ cntl->SetFailed(EPERM, "Not allowed to access builtin and Tabbed services,
try "
+ "ServerOptions.internal_port=%d instead if you're
in internal network",
+ server.options().internal_port);
+ return true;
+}
+
+// Reject accesses to non-builtin services arriving at
ServerOptions.internal_port,
+// which exposes builtin services away from the public listener, it is not a
second
+// entrance to the ordinary services of the server.
+// NOTE: Same return contract and placement rules as RejectBuiltinAccess().
+// This overload is for the protocols dispatching to a service that is never
Review Comment:
The new comment is a bit hard to parse (run-on sentence) and mixes rationale
with behavior. Consider rewriting to be more explicit/atomic, e.g. state the
rule (non-builtin rejected on `internal_port`) and then separately state the
rationale (internal_port is intended only for builtin/Tabbed exposure). This
helps future maintainers understand the contract quickly.
##########
test/brpc_socket_unittest.cpp:
##########
@@ -118,6 +118,17 @@ class CheckRecycle : public brpc::SocketUser {
}
};
+// Acceptor::Join() returns as soon as Acceptor::BeforeRecycle() cleared
+// _listened_fd, which Socket::BeforeRecycled() calls several statements before
+// it closes the fd, so the fd may still be open when Join() returns.
+void WaitForClosedFd(int fd) {
+ for (int i = 0; i < 100 && fcntl(fd, F_GETFD) != -1; ++i) {
+ bthread_usleep(10000);
+ }
+ ASSERT_EQ(-1, fcntl(fd, F_GETFD));
+ ASSERT_EQ(EBADF, errno);
Review Comment:
The retry loop assumes `fcntl(fd, F_GETFD)` only transitions from success to
`EBADF`. To keep this test robust against transient syscall failures (e.g.,
interruption), consider explicitly retrying when `fcntl` fails with `errno ==
EINTR` and only asserting `EBADF` once you’ve observed a stable invalid-fd
state (or after timeout). This reduces the chance of flaky failures under
signal-heavy test environments.
--
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]
