Repository navigation
Document TryBeginInvoke instead of the BeginInvoke the library doesn't have [patch] - #90
Open
matt-edmondson wants to merge 1 commit into
Open
matt-edmondson wants to merge 1 commit into
matt-edmondson wants to merge 1 commit into
Conversation
…t have [patch] The README's two headline async samples called invoker.BeginInvoke, and the API reference listed BeginInvoke plus IsInvokerThread and HasPendingInvokes properties. None of the three exists, so copying either sample failed with CS1061. The "Custom Thread Identification" sample overrode an IsOwnerThread method that doesn't exist either: the owner thread is fixed by whichever thread constructs the Invoker. The samples and the methods table now use TryBeginInvoke and say that it returns false, dropping the action, when the bounded queue is full. The properties table is gone. The Asynchronous Invocation sample now pumps DoInvokes until its background task finishes, rather than awaiting that task first, which would hang on its own InvokeAsync. The custom-thread section is replaced by one on choosing the owner thread and the beginInvokeCapacity constructor argument. ReadmeTests checks the README against Invoker by reflection: every invoker./_invoker. member in a C# sample, every override, and every row of the API reference with its plain return type. It also runs the Asynchronous Invocation sample's pattern under a timeout. Fixes #70 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017vfJW5rmwEcdN9nYifxAwK
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #70
This is the issue's option 1, the docs-only fix that the triage said to do right away. Adding
IsInvokerThreadandHasPendingInvokes(option 2) is a separate API decision, and this PR doesn't make it.What changed in README.md
BeginInvoke(...)→TryBeginInvoke(...), with thefalse(queue full, action dropped) case handled.Task.Runbefore callingDoInvokes(), but that task was itself awaiting anInvokeAsyncthat onlyDoInvokes()can complete. That's the same wrong-thread pattern as Invoker.Demo hangs forever: afterawait Task.Run(DoInvokes), Main resumes on a pool thread and its next Invoke() waits on a pump nobody calls #69. The sample now pumpsDoInvokes()until the background task finishes.IsInvokerThread,HasPendingInvokes) is gone. TheBeginInvoke → voidrow is nowTryBeginInvoke → bool, with its queue-full and owner-thread behaviour.protected virtual IsOwnerThread()that doesn't exist. The owner thread is private and fixed at construction. That section is replaced by Choosing the Owner Thread and Queue Capacity, which explains that the constructing thread is the owner and documents thebeginInvokeCapacityconstructor argument.Test
Invoker.Test/ReadmeTests.cschecks the README againstInvokerby reflection:invoker.X/_invoker.Xin a C# sample is a public memberoverridein a sample names an overridable memberInvokerAPI reference exists, and plain return types (void,bool,Task) match an overloadI ran the new tests against the old README and three failed:
With the new README, the full suite passes (44/44).
Overlap with open PRs
#87 adds
ValueTaskrows right next to theBeginInvokerow this PR replaces, so whichever merges second gets a one-line conflict in the methods table. The fix is to keep both the new rows and theTryBeginInvokerow. #88 edits theBeginInvokeFailedevent row, which this PR doesn't touch.Generated by Claude Code