Tcl Source Code

The following script should progressively report that it is writing to the channel, and eventually complete. Instead it hangs after the first write to the channel:

#! /usr/bin/env tclsh

namespace eval rc {
	namespace export *
	namespace ensemble create

	proc initialize {chan mode} {
		namespace eval chan_$chan {
			variable buffer {}
			variable watch {}
			variable writetask {}
		}
		return {initialize finalize watch write}
	}


	proc finalize chan {
		namespace delete chan_$chan
	}


	proc watch {chan spec} {
		namespace upvar chan_$chan watch watch writetask writetask
		set watch $spec
		after cancel $writetask
		if {{write} in $spec} {
			set writetask [after 0 [list after idle [list ::chan postevent $chan write]]]
		}
		return
	}


	proc write {chan data} {
		namespace upvar chan_$chan buffer buffer watch watch \
			writetask writetask
		append buffer $data
		after cancel writetask
		if {{write} in $watch} {
			set writetask [after 0 [
				list after idle [list ::chan postevent $chan write]]]
		}
		return [string length $data]
	}
}

set chan [chan create write [namespace which rc]]
chan configure $chan -blocking 0
coroutine c1 apply [list chan {
	after 0 [list [info coroutine]] 
	yield
	chan event $chan writable [list [info coroutine]]
	while {[incr i] < 1000000}  {
		yield
		puts [list writing to $chan $i]
		puts $chan $i
	}
	set [namespace current]::done 1
} [namespace current]] $chan
vwait [namespace current]::done

This ticket is separate from that techinical note, and illustrates a real reproducible problem. Therefore it should remain open until fixed.

Since the TCT, in the online meeting from 11-11-2024, decided to close all tickets related to this Technical Noted, and also decided to no longer allow relicensed code in Tcl official repositories (See: [560b27abdac4abe3]), this ticket can be closed.

Thanks, Ashok, for providing a working versions of Nathan's non-working version, and pointing out the reason.

This fix isn't about anything which would require a TIP. It's only about making sure an event posted by a refchan doesn't get swallowed.

Nope. See the delay at the top which allows control of how often the event is scheduled. It's 0 because your code had it as 0. In general, if a channel is capable of continuously sinking data, it is up to the channel to decide how often to generate events. In your original C implementation, it was continuously generating events (a) IRRESPECTIVE of channel state without giving the refchan control of (b) whether it needed to or not and (c) without giving a chance for idle handlers to run (the last of which you *may* have fixed).

Now, perhaps your latest code is doing something in addition. I dunno. As I said, I don't have time to review for 9.0, and think it is risky at this stage to include without a review. The crashes in CI are proof as to why, whether you fix them or not. If someone else can review and finds it worthy, fine. Else 9.0.1.

And your mention of the time you have spent correcting missteps is exactly why significant changes need to be reviewed before commits!

To summarize: Implementing a continuously-self-scheduling timer at the script level is a bad hack to overcome a bad implementation. This should be fixed.

Ashok, the "working" version of the implementation that you just proposed is exactly the strategy you are advocating against: Continuous event posting on a timer that just keeps rescheduling itself. In contrast, the fix that I produced at the C level does not do this. Instead, it just takes care to propagate the signal that is sent from the refchan by not letting it get swallowed just because an internal buffer got in the way. The issue that is illustrated in this ticket is that the C implementation currently swallows that signal. It's entirely possible to fix this at the C level so that postevent signals sent by a refchan do not get swallowed.

I could also complain about "orders of magnitude" of time that I've spent over the years correcting missteps that have been made in Tcl (ahem, the alphabet for strings for Tcl 9, or the numerous bugs that I make sure to diagnose and fix before even reporting them so that others don't have to wast their time on them), but I'm not going to engage in such bellyaching.

namespace eval rc {
    namespace ensemble create
    namespace export *
    # Change to taste depending on how much CPU you want to hog
    variable delay 0

    proc finalize {chan args} {
        namespace upvar c_$chan timer timer buffer buffer
        catch {after cancel $timer}
        puts "Finalize: Have [string length $buffer] bytes."
        namespace delete c_$chan
    }

    proc initialize {chan args} {
        namespace eval c_$chan {}
        namespace upvar c_$chan watching watching timer timer buffer buffer
        set watching {}
        set buffer {}
        return {finalize initialize watch write}
    }

    proc watch {chan spec} {
        namespace upvar c_$chan watching watching
        set watching $spec
        update $chan
    }

    proc write {chan data} {
        namespace upvar c_$chan buffer buffer
        append buffer $data
        return [string length $data]
    }

    proc update {chan} {
        namespace upvar c_$chan watching watching timer timer
        variable delay
        catch {after cancel $timer}
        if {"write" in $watching} {
            set timer [after idle after $delay \
                           [namespace code [list post $chan]]]
        }
    }

    proc post {chan} {
        variable delay
        namespace upvar c_$chan watching watching timer timer
        if {"write" in $watching} {
            set timer [after idle after $delay \
                           [namespace code [list post $chan]]]
            chan postevent $chan write
        }
    }
}

set chan [chan create write [namespace which rc]]
chan configure $chan -blocking 0
coroutine c1 apply [list chan {
    after 0 [list [info coroutine]]
    yield
    chan event $chan writable [list [info coroutine]]
    while {[incr i] < 1000000}  {
        yield
        puts [list writing to $chan $i]
        puts $chan $i
    }
    set [namespace current]::done 1
} [namespace current]] $chan
vwait [namespace current]::done
close $chan

You are right, it's not a "risk", it's an actual failure:

https://github.com/tcltk/tcl/actions/runs/10383146736/job/28747514350

chanio.test
Test file error: child killed: segmentation violation
...
io.test
Test file error: child killed: segmentation violation

(I took the freedom to start a CI build of your branch)

For all other questions you might want to ask, you can find the answer here (there are some additions by Andreas Kupries at the end of this document)

Jan, can you provide some details on what you think the risk is?

In my judgement, the risk putting this into 9.0 is higher than the benefit. I'll bring this up next TTT meeting, I will report the decision here.

The core team should take care not to become a bottleneck to needed contributions.

This fix is not the same as those that were previously argued, and the example that demonstrates the issue is also not the same. The previous arguments were about 1) whether an asynchronous channel should immediately signal writable when a channel is opened, and 2) whether postevent is asynchronous. This demo does not intersect with those concerns, and this fix touches on neither of those issues. It only resolves the issue reported in this ticket. If someone doesn't raise a specific objection to this particular fix, it should be merged into trunk.

Just so we do not go back and forth on this, I am not going to argue whether this is a bug or the fix is appropriate if so, and so on. I'm simply stating my opinion that this is to be revisited post 9.0 release. Also marking the bug open accordingly so it does not get lost.

What "Version"? Do 8.6 (and 8.7) have the same problem too?

@ashok, @sergey, your comments please?

When was it decided that TCT members review each change? That was never the case before.

Moved to bug-3968086595 branch, so it can be properly reviewed (by a TCT member), before being merged to "trunk" and "core-8-branch".

Fixed in [ab87ef464e12c190].