Copilot commented on code in PR #3525:
URL: https://github.com/apache/brpc/pull/3525#discussion_r3958367180
##########
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:
`PickUnusedPort()` / `StartWithInternalPort()` are duplicated in multiple
test files in this PR. To reduce maintenance cost and keep retry logic
consistent, consider moving these helpers into a shared test utility (e.g., a
`test/util.*` used across unit tests) and reuse it from all tests that need an
`internal_port`.
##########
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:
Polling `fcntl(fd, F_GETFD)` by numeric fd can be racy because the fd number
may be reused by another thread after the listening socket is closed. In that
case `fcntl()` can keep succeeding and the test may flake. Consider removing
the fd-number-based assertion (rely on `messenger->listened_fd() == -1`), or
add a deterministic synchronization point to the production code (e.g., signal
after the actual `close()` completes) and wait on that instead of probing the
fd.
##########
docs/en/server.md:
##########
@@ -680,9 +680,19 @@ Builtin services are useful, on the other hand include a
lot of internal informa
- Set internal port. Set ServerOptions.internal_port to a port which can
**only be accessible from internal**. You can view builtin services via
internal_port, while accesses from the public port (the one passed to
Server.Start) should see following error:
```
- [a27eda84bcdeef529a76f22872b78305] Not allowed to access builtin services,
try ServerOptions.internal_port=... instead if you're inside internal network
+ Not allowed to access builtin and Tabbed services, try
ServerOptions.internal_port=... instead if you're in internal network
```
+ Conversely internal_port serves builtin (and Tabbed) services only, requests
for ordinary services sent to it are rejected with:
+
+ ```
+ Only builtin and Tabbed services are accessible on
ServerOptions.internal_port=..., send the request to the port passed to
Server::Start() instead
Review Comment:
The docs show raw error text, but in practice `ErrorText()` output may be
prefixed (e.g., log-id / request-id in brackets) depending on how the error is
surfaced. Consider either noting that the message may be prefixed, or
formatting examples with a placeholder prefix (e.g., `[...] <message>`) to
avoid users expecting an exact full-line match.
--
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]
