Add body content filtering to message and deadletter views#871
Add body content filtering to message and deadletter views#871simonhedlund-exs wants to merge 9 commits intopaolosalvatori:mainfrom
Conversation
- Add inline search boxes for filtering messages by body content and deadletter reason - Implement body content filtering with free text and JSON path matching - Add BodyFilterForm dialog for configuring body filters (free text, JSON path, SQL property filter) - Support case-sensitive toggle and auto-discovered JSON paths - Cache message bodies for filtering performance - Apply to both queue (HandleQueueControl) and subscription (HandleSubscriptionControl) views
|
Claude finished @SeanFeldman's task in 2m 10s —— View job Code Review
Good feature idea — body content filtering is a real pain point for Service Bus debugging. The overall approach (inline search box + a richer dialog for JSON/property filters) is sensible. There are several issues that should be addressed before merging, ranging from a cache stale-data bug to significant code duplication. Bugs1.
2. In found = GetJsonValueAtPath(item, part); // passes only current segmentThis should pass the remaining path, not just Significant Issues3. Massive code duplication between
4. Custom JSON parser instead of The project already uses
Replace 5.
6. var tooltip = new ToolTip();
tooltip.SetToolTip(txt, "...");
// tooltip goes out of scope, never disposed
7. UI-blocking body reads when opening the filter dialog
Minor Issues8. 9. Placeholder text pattern is fragile Using 10. Both "Apply" and "Clear All" close the dialog with 11. The class is marked 12. Behavior change to The diff introduces a 13. private string messagesBodyFreeTextFilter = default!; // default! for string is nullThis suppresses the nullable warning but the field can be Summary
The core feature logic is sound, but I'd recommend addressing at minimum the cache-not-cleared bug (#1), the JSON navigation bug (#2), the timer disposal (#5), and swapping the custom JSON parser for |
- Fix bug: clear messageBodyCache when messages are reloaded (paolosalvatori#1) - Fix bug: replace custom JSON parser with Newtonsoft.Json.Linq, fixing incorrect recursive path navigation in arrays (paolosalvatori#2, paolosalvatori#4) - Extract shared filtering logic to MessageBodyFilterHelper to eliminate code duplication between controls (paolosalvatori#3) - Dispose Timer and ToolTip instances in Dispose() (paolosalvatori#5, paolosalvatori#6) - Remove unused BodyFilterRemovedMessage constant (paolosalvatori#8) - Use Tag-based placeholder tracking instead of ForeColor check (paolosalvatori#9) - Add WasCleared property to BodyFilterForm for clear vs apply (paolosalvatori#10) - Remove partial keyword from BodyFilterForm (paolosalvatori#11) - Revert unrelated PeekBatch behavioral change (paolosalvatori#12) - Initialize string fields to string.Empty instead of default! (paolosalvatori#13) - Show wait cursor during body cache population (paolosalvatori#7)
|
@SeanFeldman Replaced custom JSON parser with Newtonsoft.Json.Linq |
|
Wonderful, just what I needed 😄! I checked out your branch and started filtering. However, the debugger raised the following issue;
It was kind enough to offer a fix which I accepted. I'll try to create a PR to your branch with the proposed fix in a couple of hours. |
|
@claude review again. See what could cause ContextSwitchDeadlock. |
|
Claude finished @SeanFeldman's task in 3m 12s —— View job ContextSwitchDeadlock Root Cause Analysis
CauseThe The blocking call chain on the UI threadEvery time the user types a character in the inline search box (after the 400 ms timer), or clicks the filter button, this chain executes synchronously on the WinForms STA/UI thread:
During that time any other COM component (WCF channel infrastructure, the Service Bus SDK itself) that needs to marshal a call into the STA context will queue up and eventually time out at the 60-second threshold, emitting the The same pattern occurs in the filter button click handlers in both Why the existing code doesn't trigger thisThe rest of the application loads messages synchronously on the UI thread too (no FixRecommended: Populate the cache immediately after messages load Move the In messageBodyCache.Clear();
messageBindingList = new SortableBindingList<BrokeredMessage>(brokeredMessages) { ... };
// Pre-populate body cache while we're already on the "loading" path
MessageBodyFilterHelper.EnsureMessageBodyCache(
messageBindingList, messageBodyCache,
msg => serviceBusHelper.GetMessageText(msg, MainForm.SingletonMainForm.UseAscii, out _),
writeToLog);Then remove the Do the same for the deadletter cache after each Secondary issue:
|
| Location | Issue |
|---|---|
EnsureMessageBodyCache in FilterMessages() / FilterDeadletters() |
Called on UI thread per filter invocation — blocks STA, prevents COM message pumping |
EnsureMessageBodyCache in filter button click handlers |
Same issue, explicitly acknowledged with WaitCursor but still blocks STA |
GetMessageText per message |
Creates WCF encoder factory and decodes message body — COM-aware, expensive |
The fix is to pre-populate the cache at message-load time (where blocking is already expected) rather than lazily during filtering (where it blocks the message pump).
|
Hmmm, that's a different approach that I got. In my case it put the work in an asynchronous task to offload the UI thread. If the fix Claude suggested in this thread works I think that's a better solution. @simonhedlund-exs, wanna try the the fix this link from above? |
…dlock Move EnsureMessageBodyCache calls from FilterMessages/FilterDeadletters and filter button click handlers to immediately after messageBindingList and deadletterBindingList are assigned during message loading. This prevents the UI thread from being blocked by expensive WCF GetMessageText calls during filtering, which caused a 60-second ContextSwitchDeadlock MDA warning when COM context transitions could not complete.
|
@ErikMogensen This way the cache is pre-populated while the WaitCursor is already showing during load, and filtering just reads from the Dictionary — no WCF calls, no blocking, no deadlock. Pushed in commit 6a57c5d. Let me know if you still see the ContextSwitchDeadlock after this change! |
|
@simonhedlund-exs, I ran the PR through Copilot and it found the following issues: High
Medium
Low
My comments to the above points It is hard getting a PR approved these days... |
|
@simonhedlund-exs, I used your updated PR today and it solved my problem 😃. Thanks 🙏🏻 ! |
|
I suggest to solve the first one by reading the message bodies during the first search and then storing them in a cache. |
|
I realize now that the maximum queue size is 80 GB which would break most machines if it was stored in the cache. That complicates the caching 😢 |
Remove unconditional body cache population at load time.
Bodies are now decoded on demand, on a background thread, the
first time a body filter is applied. The UI thread is never
blocked; FilterMessages/FilterDeadletters re-runs automatically
when the cache is ready via ContinueWith on the UI context.
A version counter (_messageCacheVersion) prevents stale cache
completions from a superseded message load from affecting the
current view.
The filter dialog (pictFind) uses synchronous population since
the user explicitly clicked — a brief wait is expected there.
Also fix:
- Tag set before Text in LostFocus placeholder handlers to
prevent spurious debounce timer trigger on blur
- catch {} -> catch (Exception) in BodyFilterForm JSON helpers
- Remove redundant Close() calls after setting DialogResult
- Remove dead WasCleared property from BodyFilterForm
|
@ErikMogensen Went through all the points and pushed a new commit. The biggest change is the cache strategy — you were right that pre-populating on every load was wasteful. Bodies now get decoded on a background thread only when a filter is actually applied, and the filter just re-runs automatically when it's done. Added a version counter so a slow background task from an old load can't corrupt the current view. Also fixed the LostFocus placeholder issue (was setting Text before Tag, which would fire the debounce timer on blur), swapped out the bare catch {} blocks, removed the redundant Close() calls in the dialog, and cleaned up the dead WasCleared property. Let me know if anything looks off! |
|
My Copilot says: Review:
|
…plorer into feature/body-search-filter
…tCursor, 2GB cap, DPI scaling, JsonException, WireDeadLetterReasonFilter rename
|
Hi @ErikMogensen , thanks for the feedback! Fixed everything in 949c4d3:
Let me know if anything else needs changing! |
|
Thank you @simonhedlund-exs, I ran it through my local Copilot and got the following: Code Review:
|
…plorer into feature/body-search-filter
- Remove Font leak in MessageBodyFilterHelper (GDI handle leak) - Replace manual JSON path parsing with SelectToken (handles dots in keys) - Add 2GB cache cap to sync EnsureMessageBodyCache (consistency with async) - Implement re-entrancy callback storage for better UX during cache population Resolves review comments from Erik Mogensen. Co-authored-by: Copilot <copilot@github.com>
Add explicit Close() call after setting DialogResult in BtnClear_Click for clarity. While setting DialogResult closes a modal form implicitly, the explicit Close() makes the intent clearer. Addresses review feedback paolosalvatori#7 from PR paolosalvatori#871
Implements #870