Skip to content

[bugfix] bound connect and read timeouts for Memcached and WebSocket collection - #4493

Open
SiriusTL666 wants to merge 1 commit into
apache:masterfrom
SiriusTL666:fix/collector-socket-timeouts
Open

SiriusTL666 wants to merge 1 commit into
apache:masterfrom
SiriusTL666:fix/collector-socket-timeouts

Conversation

@SiriusTL666

Copy link
Copy Markdown

What's changed?

MemcachedCollectImpl and WebsocketCollectImpl both create a bare Socket and call connect(SocketAddress) with no timeout, then read the peer's reply with no SO_TIMEOUT. When the SYN is dropped, the port is firewalled, or the peer accepts the connection but never answers, the call blocks until the OS-level TCP timeout — typically minutes — while the collection worker stays occupied and the monitor reports NO DATA.

Both classes already had a catch (SocketTimeoutException) branch, but nothing ever set a timeout, so that branch was unreachable.

Changes

  1. Memcached. socket.connect(address, timeout) plus socket.setSoTimeout(timeout).
  2. WebSocket. The same, so a peer that accepts the upgrade request but never sends the response line no longer hangs.
  3. Configurable. MemcachedProtocol and WebsocketProtocol gain an optional timeout, exposed as a monitor parameter in app-memcached.yml and app-websocket.yml (range 1000–120000 ms, default 10000).
  4. Tests. Both existing test classes gain two cases each: one asserting the configured value reaches connect(...) and setSoTimeout(...), and one asserting the 10s fallback. The WebSocket cases also assert the unreachable branch now reports UN_CONNECTABLE.

This is the same class of defect as #4492 (SSL certificate collection) and #4490 (IPMI UDP receive). I audited the rest of hertzbeat-collector-basic for Socket / DatagramSocket / DatagramChannel usage: UdpCollectImpl already sets SO_TIMEOUT, and the SSH/FTP collectors use libraries that take a timeout argument, so these two were the remaining gaps.

Checklist

  • I have read the Contributing Guide
  • I have written the necessary doc or comment.
  • I have added the necessary unit tests and all cases have passed.

Verification

  • mvn -pl hertzbeat-collector/hertzbeat-collector-basic test -Dtest=MemcachedCollectImplTest,WebsocketCollectImplTest — 8/8 pass (5 memcached, 3 websocket).
  • checkstyle:check on hertzbeat-collector-basic and hertzbeat-common-core — 0 violations.
  • The pre-existing success-path tests in both classes still pass, so the change does not affect response parsing.

No issue exists for this one; like #4492 I found it while auditing for unbounded network waits. Happy to file issues first if the project prefers that.

Add or update API

  • I have added the necessary e2e tests and all cases have passed.

No HTTP API change. The timeout monitor parameter is optional and defaults to 10s, which is bounded where the previous behaviour was unbounded.

…ket collection

Both collectors created a bare Socket and called connect(SocketAddress) with no
timeout, then read the response with no SO_TIMEOUT. A dropped SYN, a firewalled
port, or a peer that accepts the connection but never answers blocks the
collector until the OS-level TCP timeout — minutes — while the worker stays
occupied and the monitor shows NO DATA. Both classes already had a
SocketTimeoutException branch that could never fire, because nothing ever set
a timeout.

- Pass the monitor's timeout (default 10s) to connect(addr, timeout) and
  setSoTimeout for both collectors.
- Add an optional timeout parameter to app-memcached.yml and
  app-websocket.yml.
- Extend both unit tests to assert the timeout value reaches the socket, once
  configured and once falling back to the default.
@SiriusTL666

Copy link
Copy Markdown
Author

Hi maintainers, the workflows on this PR are showing action_required because this is my first contribution to this repository. Could someone approve the workflow runs when convenient? Happy to address any review comments in the meantime.

For context: linked issue is #3197, and I checked before starting that no IPMI fix PR existed. I left a note on the issue that I would step back if either of the earlier commenters is already working on it — happy to coordinate.

@SiriusTL666

Copy link
Copy Markdown
Author

While waiting for a maintainer to approve the workflows on this PR (this is my first contribution here, so they show action_required), I enabled Actions in my fork and ran the same Backend CI workflow there against this exact commit range. It passes:

https://github.com/SiriusTL666/hertzbeat/actions/runs/40178308108

Job Result
backend-build success
backend-maven-e2e success
backend-image-e2e success

That covers the Maven build, the frontend release-asset build, and the server release-layout verification, so the change compiles and the module tests pass on the project's own CI definition.

To be explicit: this is a run in my fork, not on this PR — it is evidence for the reviewer, not a substitute for the required approval. Once someone approves, the run on this PR should behave the same.

Also worth noting: in the fork run I had to touch a real (non-markdown) file to trigger the push-based workflow, since paths-ignore skips commits with no relevant changes. The PR branch itself is unchanged and clean.

Happy to address any review comments.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant