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.
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
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.
5cc58fe to
b2a1946
Compare
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.