Repository navigation
examples, apps: half close and drain at shutdown - #1305
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
5cc58fe to
b2a1946
Compare
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 for up to ten seconds, then drains. It returns WS_WANT_READ for a peer that never hung up, WS_WANT_WRITE for a disconnect still queued, WS_SUCCESS for a reset. - test.h adds WaitForChannelClose(), which runs the session until the peer's channel close, bounded at ten seconds, and returns at once on a session an error already ended. MQX gets one pass. - client.c waits for the close, keeping a late exit status, and reports an unexpected shutdown result. It exits 0 on either give-up and fails on a broken wait or a disconnect it could not send. - 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. configure checks for socketpair(), and unit.c builds these tests only when it is found, since custom-IO builds such as the STM32 Cube pack have no sockets API. - peer hangs up after the disconnect, or resets before it goes out - first send would block, the retry flushes it - peer disconnected first, nothing is sent - the drain alone sees the hang up, and sends a disconnect of ours a short send left queued - an exit status after the peer's EOF and an IGNORE is still read, and the wait returns at once on a session an error already ended
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. - the wolfssh app waits for the channel close before draining, as the client does, and fails on an error that broke the wait. - wolfsshd drained by hand after wolfSSH_free(), with no bound on a peer that never hung up. HalfCloseAndDrain() bounds it at ten seconds, and a peer that let the login grace time run out is not drained.
b2a1946 to
cae463d
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: 5 of 5 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
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.