Repository navigation
examples, apps: half close and drain at shutdown - #1305
ejohnstown wants to merge 3 commits into
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
Normal worker events can truncate queued output, and the drain does not enforce the stated ten-second wall-clock bound.
2 open findings
What changed in this PR
Adds graceful socket half-close and drain handling across clients and servers, replacing fixed worker retries.
Changes:
- Adds shared shutdown/drain helpers with platform handling.
- Updates applications and examples to use graceful draining.
- Adds socket-pair unit coverage.
| File | Description |
|---|---|
wolfssh/test.h |
Adds half-close and disconnect-drain helpers. |
tests/unit.c |
Tests disconnect and half-close paths. |
examples/sftpclient/sftpclient.c |
Uses graceful client draining. |
examples/scpclient/scpclient.c |
Uses graceful client draining. |
examples/portfwd/portfwd.c |
Drains the forwarding connection. |
examples/echoserver/echoserver.c |
Half-closes server connections. |
examples/client/client.c |
Uses disconnect-and-drain teardown. |
apps/wolfsshd/wolfsshd.c |
Adds bounded daemon connection draining. |
apps/wolfssh/wolfssh.c |
Uses shared graceful teardown. |
🧠 Review effort: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
12516a6 to
b782b43
Compare
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #1305
Scan targets checked: wolfssh-src, wolfssh-bugs
Coverage: 7 of 9 in-scope changed file(s) opened by the reviewer; not opened: examples/portfwd/portfwd.c, tests/unit.c
Findings: 1
1 finding(s) posted as inline comments (see file-level comments below)
This review was generated automatically by Fenrir. Reported findings require changes before merge.
Review tier: Lite
b782b43 to
54ec4cd
Compare
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #1305
Scan targets checked: wolfssh-src, wolfssh-bugs
Coverage: 2 of 2 in-scope changed file(s) opened by the reviewer
Fenrir result: Approved ✅
No new issues found in the changed files.
Advisory only — this automated result does not count as a GitHub approval.
Review tier: Lite
Fenrir's latest completed scan found no issues; clearing the prior automated change request.
The client and the echoserver both sent their shutdown messages and then spun wolfSSH_worker() a fixed number of times, so whether the peer's close was seen at all depended on timing. Both now half close the socket and read until the peer hangs up; the client sends a disconnect first, the echoserver does not. - test.h adds WSHUTDOWN(), a write-side shutdown() on POSIX and Windows, left undefined where the stack has no half close. - test.h adds HalfCloseAndDrain(). It flushes a queued write, then reads the raw socket, since the library reads nothing after the session ends, and gives up ten seconds in or after 256 reads. MQX gets a stub. - SendDisconnectAndDrain() sends MSG_DISCONNECT and then drains. It returns WS_WANT_READ for a peer that never hung up and WS_WANT_WRITE for a disconnect still queued. - client.c reports an unexpected shutdown result, exits 0 on either give-up and fails on a disconnect it could not send. A peer that already hung up skips straight to the close. - echoserver.c replaces its 10-attempt worker loop with the drain alone: the close exchange ended the session, and OpenSSH's client exits 255 on a disconnect that lands before it is done.
Run the helpers over a socket pair with the test playing the peer. - peer hangs up after the disconnect - first send would block, the retry flushes it - peer disconnected first, nothing is sent - the drain alone sends nothing and still sees the hang up - the drain sends a disconnect of ours a short send left queued
The remaining teardown sites spun wolfSSH_worker() ten times after wolfSSH_shutdown(), the loop the client and the echoserver just replaced. The clients now send a disconnect and drain, and wolfsshd half closes and drains like the echoserver. - sftpclient.c, scpclient.c, portfwd.c and the wolfssh app call SendDisconnectAndDrain() and treat either give-up as a clean close. - scpclient.c and the wolfssh app report an unexpected shutdown result, as the client does. - wolfsshd drained by hand after wolfSSH_free(), with no bound on a peer that never hung up. HalfCloseAndDrain() bounds it at ten seconds.
54ec4cd to
5cc58fe
Compare
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #1305
Scan targets checked: none
Unchanged since last review (not re-run): wolfssh-src, wolfssh-bugs
Fenrir result: Approved ✅
No new issues found in the changed files.
Advisory only — this automated result does not count as a GitHub approval.
Review tier: Lite
Both examples and all the apps half close the socket at shutdown and read until the peer hangs up, instead of spinning
wolfSSH_worker()a fixed number of times. Clients send a disconnect first; servers do not, since OpenSSH's client exits 255 on a disconnect that lands before it is done.test.haddsHalfCloseAndDrain()andSendDisconnectAndDrain(), with unit coverage over a socket pair.wolfsshd's teardown is now bounded at ten seconds for a peer that never hangs up.