Skip to content

Conversation

@Namoshek
Copy link
Collaborator

@Namoshek Namoshek commented Dec 9, 2025

Currently, we only check for EOF when reading in blocking mode. The client uses non-blocking reads most of the time though, such that we need to also check for EOF in those cases.

This fix is in direct response to php-mqtt/laravel-client#73 which shows that a disconnect is currently only detected through the keep alive pings and not by reading from the socket.

Currently, we only check for EOF when reading in blocking mode. The client uses non-blocking reads most of the time though, such that we need to also check for EOF in those cases.
Copilot AI review requested due to automatic review settings December 9, 2025 19:31
@Namoshek Namoshek added the bug Something isn't working label Dec 9, 2025
@github-actions
Copy link

github-actions bot commented Dec 9, 2025

Dependency Review

✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.

Scanned Files

None

@Namoshek Namoshek merged commit 0021340 into master Dec 9, 2025
25 checks passed
@Namoshek Namoshek deleted the fix/check-for-eof-when-reading-from-socket branch December 9, 2025 19:37
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR addresses a bug where EOF (End of File) conditions on the socket were only detected during blocking reads, not during non-blocking reads which are used most of the time by the client. The fix adds an upfront EOF check in the readFromSocket() method to detect broker-initiated disconnections immediately rather than only through keep-alive pings.

Key Changes:

  • Added EOF check at the beginning of readFromSocket() method to detect socket closure in both blocking and non-blocking modes
  • Throws DataTransferException with appropriate error message when EOF is detected

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

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

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants