Skip to content

fix: preserve WLAN timeout across reconnects - #311

Open
YashAnand69 wants to merge 2 commits into
fossasia:mainfrom
YashAnand69:fix/wlan-timeout-reconnect
Open

YashAnand69 wants to merge 2 commits into
fossasia:mainfrom
YashAnand69:fix/wlan-timeout-reconnect

Conversation

@YashAnand69

@YashAnand69 YashAnand69 commented Oct 9, 2026 •

Copy link
Copy Markdown

Changing WLANHandler.timeout updates its socket but leaves the cached timeout unchanged. The getter returns the old setting, and after disconnecting, connect() creates a replacement socket using that old timeout.

Cache the requested value after socket.settimeout succeeds. Reconnects retain the setting, and rejected values leave the previous configuration intact. This is separate from #294's partial-transfer and closed-connection fixes.

Validation:

  • Eleven hardware-independent cases: six fail on unchanged main and five controls pass. Real loopback TCP sockets verify disconnect/reconnect, updated getter/socket values, normal transfers, rejected values and failed-handshake cleanup. Only the device-version handshake is replaced.
  • Selected WLAN/CLI/MQ135/Pico tests: 44 passed. WLAN module line coverage reaches 100%; this is not a full hardware-integration coverage claim.
  • Configured Black, Flake8, Bandit and pydocstyle checks pass. Wheel/source distribution and documentation build pass; docs retain seven existing warnings.
  • No physical PSLab, ESP8266 or full hardware record/playback validation was performed.

No matching timeout-state issue or overlapping open PR found. Developed and tested with OpenAI Codex assistance.

Summary by Sourcery

Preserve WLAN timeout configuration consistently across socket updates and reconnects.

New Features:

  • Allow WLAN socket timeouts to be disabled by configuring them as None.

Bug Fixes:

  • Preserve updated WLAN timeout settings in the handler and across reconnects while leaving the previous configuration unchanged when invalid values are rejected.

Tests:

  • Add hardware-independent loopback tests covering timeout updates, reconnect behavior, invalid values, transfers, and failed-handshake cleanup.

Summary by CodeRabbit

  • New Features
    • WLAN connections can now be configured without a socket timeout by setting the timeout to None.
  • Bug Fixes
    • Timeout changes are retained when a closed WLAN connection reconnects, keeping the configured behavior consistent.
  • Tests
    • Added coverage for WLAN timeout settings, reconnections, data transfers, invalid values, and failed connection attempts.

@sourcery-ai

sourcery-ai Bot commented Oct 9, 2026 •

Copy link
Copy Markdown

Reviewer's Guide

Fix WLAN timeout state so successful updates are reflected by the getter and retained across reconnects, while invalid values preserve the previous configuration; add loopback and socket-based regression tests covering timeout synchronization, reconnects, transfers, validation, and handshake cleanup.

Sequence diagram for WLAN timeout persistence across reconnects

sequenceDiagram
    participant Client
    participant WLANHandler
    participant Socket

    Client->>WLANHandler: timeout = value
    WLANHandler->>Socket: settimeout(value)
    alt settimeout succeeds
        WLANHandler->>WLANHandler: cache requested timeout
        Client->>WLANHandler: timeout
        WLANHandler-->>Client: updated timeout
        Client->>WLANHandler: disconnect()
        WLANHandler->>Socket: close()
        Client->>WLANHandler: connect()
        WLANHandler->>Socket: create replacement socket
        WLANHandler->>Socket: settimeout(cached timeout)
    else settimeout rejects value
        Socket-->>WLANHandler: error
        WLANHandler-->>Client: error
        Client->>WLANHandler: timeout
        WLANHandler-->>Client: previous timeout
    end
Loading

File-Level Changes

Change Details Files
Keep the requested WLAN timeout synchronized with the active socket and preserve it when reconnecting.
  • Update the cached timeout only after the socket accepts the new value.
  • Ensure replacement sockets inherit the updated timeout through the existing connection setup.
  • Leave both cache and socket unchanged when timeout validation fails.
pslab/connection/wlan.py
Add hardware-independent regression coverage for timeout behavior and connection lifecycle handling.
  • Test getter and socket values for initial, updated, and rejected timeout settings.
  • Verify updated timeouts survive disconnect/reconnect and normal read/write transfers.
  • Verify failed handshakes close the socket while retaining existing cleanup behavior.
tests/test_wlan_timeout.py

Tips and commands

Interacting with Sourcery

  • Trigger a new review: Comment @sourcery-ai review on the pull request.
  • Continue discussions: Reply directly to Sourcery's review comments.
  • Generate a GitHub issue from a review comment: Ask Sourcery to create an
    issue from a review comment by replying to it. You can also reply to a
    review comment with @sourcery-ai issue to create an issue from it.
  • Generate a pull request title: Write @sourcery-ai anywhere in the pull
    request title to generate a title at any time. You can also comment
    @sourcery-ai title on the pull request to (re-)generate the title at any time.
  • Generate a pull request summary: Write @sourcery-ai summary anywhere in
    the pull request body to generate a PR summary at any time exactly where you
    want it. You can also comment @sourcery-ai summary on the pull request to
    (re-)generate the summary at any time.
  • Generate reviewer's guide: Comment @sourcery-ai guide on the pull
    request to (re-)generate the reviewer's guide at any time.
  • Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
    pull request to resolve all Sourcery comments. Useful if you've already
    addressed all the comments and don't want to see them anymore.
  • Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
    request to dismiss all existing Sourcery reviews. Especially useful if you
    want to start fresh with a new review - don't forget to comment
    @sourcery-ai review to trigger a new review!

Customizing Your Experience

Access your dashboard to:

  • Enable or disable review features such as the Sourcery-generated pull request
    summary, the reviewer's guide, and others.
  • Change the review language.
  • Add, remove or edit custom review instructions.
  • Adjust other review settings.

Getting Help

@coderabbitai

coderabbitai Bot commented Oct 9, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: defaults
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 0c9d86f1-358d-4a3e-b00b-9fbec9bb0438

📥 Commits

Reviewing files that changed from the base of the PR and between 08de0d0 and f9481e7.


📒 Files selected for processing (2)
  • pslab/connection/wlan.py
  • tests/test_wlan_timeout.py

Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 1 remain after this review.



📝 Walkthrough

Walkthrough

WLANHandler accepts None as a timeout value and stores timeout updates for use when reconnecting. Tests cover timeout settings, reconnects, socket transfers, invalid values, and failed handshakes.

Changes

WLAN timeout behavior

Layer / File(s) Summary
Timeout state and regression coverage
pslab/connection/wlan.py, tests/test_wlan_timeout.py
The constructor and timeout property accept optional timeout values. The setter stores valid updates after applying them to the socket. Tests cover timeout initialization and updates, reconnects, invalid values, socket transfers, and failed handshakes.

Priority: ⬇️ Low

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix


Merge Risk

Merge Risk: ⚪ Minimal · up to f9481

The timeout change appears mergeable after normal checks; no actionable merge-blocking risk was identified.

Security Architecture Review

Security architecture risk: 🔵 Low · up to f9481

The change is narrowly scoped and preserves the finite default timeout. Explicitly disabling the timeout now also affects subsequent connections, so applications choosing that option may wait indefinitely for an unresponsive peer. Existing connection checks remain unchanged, and no introduced security finding was established.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

  • inferred — The directly changed exposure is a handler's replacement sockets inheriting the caller's timeout choice. With None, an unresponsive peer can leave blocking operations waiting indefinitely after reconnect as well as before it. Wider application impact depends on the consumer's execution model, which was not supplied.

Trust Boundaries and Controls

  • observed — The changed setter obtains timeout values from the local caller, not from peer responses. Host, port, the finite default, and handshake invocation are unchanged. The inspected change does not bypass the existing version check or introduce credential handling.

Resilience and Maintainability Implications

  • observed — Handshake exceptions still close the socket and re-raise while retaining timeout configuration for recovery. The network connect call remains outside that cleanup block; its failure behavior is pre-existing rather than introduced by timeout retention.
  • inferred — The socket update, cache assignment, and socket replacement are separate unsynchronized operations. Overlapping calls or interruption can split cached and live timeout state. Concurrent-use support and a resulting security-relevant exposure were not established, so this remains a limitation rather than a retained concern.



🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage Warning Docstring coverage is 75.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 12 functions across 2 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check Passed The title clearly and concisely describes the main change: preserving the WLAN timeout across socket reconnects.
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.


  • Fix all pre-merge checks with AI
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR


  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@sourcery-ai sourcery-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Hey - I've found 1 issue

Prompt for AI Agents
Please address the comments from this code review:

## Individual Comments

### Comment 1
<location path="pslab/connection/wlan.py" line_range="49" />
<code_context>
     @timeout.setter
     def timeout(self, value: float) -> None:
         self._sock.settimeout(value)
+        self._timeout = value

     def connect(self) -> None:
</code_context>
<issue_to_address>
**Indefinite timeout has wrong type**

When a caller sets the timeout to `None`, the getter returns `None` despite the constructor, setter, and getter declaring `float`; callers using the value as a float get a `TypeError`, and type checkers cannot represent the supported indefinite-timeout value.

Update the constructor, setter, and getter annotations to allow `None` as well as `float`.
</issue_to_address>

Sourcery assessment

Approval pending. 1 finding to address first.

Blocking findings: pslab/connection/wlan.py:49


Sourcery is free for open source - if you like our reviews please consider sharing them ✨

Comment thread pslab/connection/wlan.py

@sourcery-ai sourcery-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Sourcery assessment

Approved.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant