Consider runtime warning when WeaveHandle is dropped without completion/cancellation #141

New issue

Closed

opened 2025-12-30 15:25:20 +00:00 by navicore · 1 comment

navicore commented

2025-12-30 15:25:20 +00:00

(Migrated from github.com)

Background

PR #138 documents that weaves must be either:

Resumed until completion (has_more returns false)
Explicitly cancelled with strand.weave-cancel

If neither happens and the WeaveHandle is dropped, the spawned coroutine hangs forever.

Current State

The limitation is well-documented in weave.rs:

**Important:** Weaves must either be resumed until completion OR explicitly
cancelled with `strand.weave-cancel`. Dropping a WeaveHandle without doing
either will cause the spawned coroutine to hang forever waiting on resume_chan.

Problem

While documented, this is a silent resource leak that's easy to introduce accidentally:

: example ( -- )
  [ my-generator ] strand.weave
  0 strand.resume
  if
    io.write-line  # Process first value
    drop           # DROP THE HANDLE - LEAK!
  else
    drop drop
  then
;

Options to Consider

Option A: Auto-cancel on Drop (Implicit)

impl Drop for WeaveCtx {
    fn drop(&mut self) {
        let _ = self.resume_chan.sender.send(Value::Int(CANCEL_SENTINEL));
    }
}

Pro: No leaks possible
Con: Hides programming errors, unexpected behavior

Option B: Warning on Drop (Diagnostic)

Track completion state in WeaveCtx, emit warning if dropped without completion:

impl Drop for WeaveCtx {
    fn drop(&mut self) {
        if !self.completed {
            eprintln!("Warning: WeaveHandle dropped without completion or cancellation");
        }
    }
}

Pro: Alerts developer to bug
Con: Runtime overhead, noisy in some valid patterns

Option C: Debug-only assertion

#[cfg(debug_assertions)]
impl Drop for WeaveCtx {
    fn drop(&mut self) {
        debug_assert!(self.completed, "WeaveHandle dropped without cleanup");
    }
}

Pro: Catches bugs in development
Con: Silent in release builds

Lint rule to detect strand.weave without matching strand.weave-cancel or completion loop.
Pro: Catches at edit time
Con: Complex flow analysis, may have false positives

Option E: Keep documentation only (Current)

Pro: Simple, no runtime overhead
Con: Easy to miss in practice

Recommendation

Start with Option C (debug assertion) + Option D (LSP lint). This provides:

Development-time catching via assertion
Edit-time warning via LSP
No production overhead

Acceptance Criteria

Decide on approach
Implement chosen solution
Add test demonstrating the behavior
Update documentation if needed

PR #138 (weave implementation)
Issue #131 (original generator request)
Future: LSP lint rules for weave safety

## Background PR #138 documents that weaves must be either: 1. Resumed until completion (`has_more` returns false) 2. Explicitly cancelled with `strand.weave-cancel` If neither happens and the WeaveHandle is dropped, the spawned coroutine hangs forever. ## Current State The limitation is well-documented in `weave.rs`: ``` **Important:** Weaves must either be resumed until completion OR explicitly cancelled with `strand.weave-cancel`. Dropping a WeaveHandle without doing either will cause the spawned coroutine to hang forever waiting on resume_chan. ``` ## Problem While documented, this is a silent resource leak that's easy to introduce accidentally: ```seq : example ( -- ) [ my-generator ] strand.weave 0 strand.resume if io.write-line # Process first value drop # DROP THE HANDLE - LEAK! else drop drop then ; ``` ## Options to Consider ### Option A: Auto-cancel on Drop (Implicit) ```rust impl Drop for WeaveCtx { fn drop(&mut self) { let _ = self.resume_chan.sender.send(Value::Int(CANCEL_SENTINEL)); } } ``` **Pro:** No leaks possible **Con:** Hides programming errors, unexpected behavior ### Option B: Warning on Drop (Diagnostic) Track completion state in WeaveCtx, emit warning if dropped without completion: ```rust impl Drop for WeaveCtx { fn drop(&mut self) { if !self.completed { eprintln!("Warning: WeaveHandle dropped without completion or cancellation"); } } } ``` **Pro:** Alerts developer to bug **Con:** Runtime overhead, noisy in some valid patterns ### Option C: Debug-only assertion ```rust #[cfg(debug_assertions)] impl Drop for WeaveCtx { fn drop(&mut self) { debug_assert!(self.completed, "WeaveHandle dropped without cleanup"); } } ``` **Pro:** Catches bugs in development **Con:** Silent in release builds ### Option D: Static analysis via LSP lint (See related issue) Lint rule to detect `strand.weave` without matching `strand.weave-cancel` or completion loop. **Pro:** Catches at edit time **Con:** Complex flow analysis, may have false positives ### Option E: Keep documentation only (Current) **Pro:** Simple, no runtime overhead **Con:** Easy to miss in practice ## Recommendation Start with **Option C** (debug assertion) + **Option D** (LSP lint). This provides: - Development-time catching via assertion - Edit-time warning via LSP - No production overhead ## Acceptance Criteria - [ ] Decide on approach - [ ] Implement chosen solution - [ ] Add test demonstrating the behavior - [ ] Update documentation if needed ## Related - PR #138 (weave implementation) - Issue #131 (original generator request) - Future: LSP lint rules for weave safety

navicore commented

2025-12-30 16:56:38 +00:00

(Migrated from github.com)

Closing as won't fix after analysis.

Rationale:

The implementation cost is high relative to the benefit:

WeaveHandle and WeaveCtx are the same type (Value::WeaveCtx) sharing Arc'd channels
Auto-cancel on Drop would require a new Value variant, FFI changes, and complex Drop logic to distinguish caller handle from coroutine context

What we have instead:

Lint rule: strand.weave drop → error (catches immediate drops)
Lint rule: strand.resume drop drop drop → warning (catches unchecked resumes)
Clear documentation in weave.rs
Issue #142 will add Phase 2 data-flow analysis for smarter detection

The lint rules catch obvious cases at edit time (better than runtime warnings), and #142 will provide more sophisticated static analysis. After #139, even if a weave hangs, it doesn't cause UB - the coroutine just blocks harmlessly.

Closing as won't fix after analysis. **Rationale:** The implementation cost is high relative to the benefit: - `WeaveHandle` and `WeaveCtx` are the same type (`Value::WeaveCtx`) sharing Arc'd channels - Auto-cancel on Drop would require a new Value variant, FFI changes, and complex Drop logic to distinguish caller handle from coroutine context **What we have instead:** - Lint rule: `strand.weave drop` → error (catches immediate drops) - Lint rule: `strand.resume drop drop drop` → warning (catches unchecked resumes) - Clear documentation in weave.rs - Issue #142 will add Phase 2 data-flow analysis for smarter detection The lint rules catch obvious cases at edit time (better than runtime warnings), and #142 will provide more sophisticated static analysis. After #139, even if a weave hangs, it doesn't cause UB - the coroutine just blocks harmlessly.