Skip to content

M1I05: SocketProxyInterface + ReusePortProxy#61

Merged
s2x merged 2 commits into
mainfrom
feature/m1i05-socketproxyinterface-reuseportproxy
May 1, 2026
Merged

M1I05: SocketProxyInterface + ReusePortProxy#61
s2x merged 2 commits into
mainfrom
feature/m1i05-socketproxyinterface-reuseportproxy

Conversation

@s2x
Copy link
Copy Markdown
Contributor

@s2x s2x commented May 1, 2026

Summary

File Action
src/Server/Socket/SocketProxyInterface.php Create — interface with createSocket(), accept(), isSupported()
src/Server/Socket/ReusePortProxy.php Create — SO_REUSEADDR + SO_REUSEPORT proxy
tests/Server/Socket/ReusePortProxyTest.php Create — unit tests

Acceptance criteria

  • SocketProxyInterface defines 3 methods with correct signatures
  • ReusePortProxy implements the interface, uses SO_REUSEADDR + conditional SO_REUSEPORT
  • isSupported() returns false on Windows or when constant undefined
  • Tests pass (or skip on unsupported platforms)

Closes #7

- SocketProxyInterface with createSocket(), accept(), isSupported()
- ReusePortProxy implementing SO_REUSEADDR + SO_REUSEPORT
- Unit tests with platform-aware skipping
Copy link
Copy Markdown
Contributor Author

@s2x s2x left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review Summary — Round 1

# Severity File:Line Finding
1 MEDIUM ReusePortProxyTest.php:27 Unnecessary skip in testCreateSocketReturnsSocket
2 LOW ReusePortProxyTest.php:42 Unnecessary skip in testAcceptReturnsConnection

Follow-up Observations

  • Unused $protocol parameter: ReusePortProxy::createSocket() accepts ProtocolType $protocol but always creates AF_INET, SOCK_STREAM, SOL_TCP sockets regardless. Functionally correct for current types (TCP, HTTP, WS all run over TCP), but a future non-TCP protocol would silently get wrong socket type. Consider @param docs or a match on protocol.
  • 0.0.0.0 for client connect: testAcceptReturnsConnection connects to $address from socket_getsockname which returns 0.0.0.0. Existing tests (e.g., SocketTest) bind to 127.0.0.1 explicitly. Consider binding to 127.0.0.1 in the proxy or overriding in the test for consistency.
  • No error-path tests: No test covers the SocketCreationException path (e.g., when socket_create fails).

What's Fine ✔️

  • Interface signatures match spec exactly
  • isSupported() implementation matches spec (defined('SO_REUSEPORT') && PHP_OS_FAMILY !== 'Windows')
  • Conditional SO_REUSEPORT logic is correct
  • Test covers interface implementation, socket creation, accept, and isSupported
  • No security issues (no secrets, PII, injection vectors)
  • All acceptance criteria from issue #7 and milestone doc satisfied

Comment thread tests/Server/Socket/ReusePortProxyTest.php Outdated
Comment thread tests/Server/Socket/ReusePortProxyTest.php Outdated
Copy link
Copy Markdown
Contributor Author

@s2x s2x left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review Summary — Round 2

# Severity Status Finding
1 MEDIUM ✅ Fixed testCreateSocketReturnsSocket had unnecessary isSupported() skip
2 LOW ✅ Fixed testAcceptReturnsConnection had same unnecessary skip

Follow-up Observations

  • $protocol param unused in createSocket — always creates TCP sockets. When HTTP/WebSocket support is added to proxy implementations, this parameter will be used for protocol-specific socket creation.
  • Reflection helpergetSocketResource() could be extracted to a test trait if similar helpers are needed in other proxy tests.

What's Fine ✅

  • All 4 CI checks pass (lint + tests on PHP 8.2–8.5)
  • Interface signatures match spec exactly: createSocket(), accept(), isSupported()
  • ReusePortProxy implements SO_REUSEADDR + conditional SO_REUSEPORT correctly
  • isSupported() matches spec: defined('SO_REUSEPORT') && PHP_OS_FAMILY !== 'Windows'
  • All issue #7 acceptance criteria satisfied
  • No security issues, secrets, PII, or injection vectors
  • Code follows existing project patterns

⚠️ Self-review — awaiting human approval.

@s2x s2x merged commit b85b8b0 into main May 1, 2026
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

M1I05: SocketProxyInterface + ReusePortProxy

1 participant