Update output queueing and batching logic #1510

stefan-toubia · 2022-01-27T20:03:21Z

What has Changed?

This refactors the logic for queueing and writing to the output window. #1509 points out that certain conditions can cause the output window to get stuck in isEditing mode and cause the queue to grow indefinitely.

The main change in this PR is to queue all appended output to the new EditQueue to try and eliminate some of these edge conditions causing the output to break.

This is still a draft because I still have some questions around the previous batching logic and OnAppendedCallback

Fixes #1189
Fixes #1509

My Calva PR Checklist

I have:

Ping @PEZ, @bpringe

stefan-toubia · 2022-01-27T20:17:49Z

src/results-output/results-doc.ts

+
+    // Note that these onAppended callbacks are batched and called after all
+    // current batched entries have been written.
+    onAppendedCallbacks.forEach((onAppended) =>


It wasn't immediately apparent to me what the intent was behind the previous batching behavior, so I held off on addressing it until I could clarify. Previously the batches had extra partitioning logic that ensured at most one onAppendedCallback existed in a batch. I could see two reasons for this -

It's important to call onAppended immediately after that specific text was written and before any next entries are written

It's important to provide the exact Location for that text entry

It looks like these callbacks are (exclusively?) used by evaluate to add selection decoration, so I'm under the impression that it's really (2) that we want here. Regardless, I think both cases do not require special batching logic, instead I can rearrange the contents of handleNextOutputQueueBatch to call each queued onAppended item immediately after writing it instead of after writing the whole batch.

My brain is fried right now so I will refrain from having any input on this right now. And I think @bpringe is the one who knows most about this. anyway. Also, iirc, @brdloush has been thinking (and also doing) things about this whole batching in general.

Yeah, I believe the batching was added by @brdloush, so let's see what he has to say about this.

No response from @brdloush, I'd like to squash this bug. I can rework this to maintain the existing batching logic. The important part of this PR is removing applyingEdit. WDYT @PEZ and @bpringe, do you see any blockers?

I'd like to test this and review this more thoroughly when I have more time, unless @PEZ can get to it before me.

stefan-toubia · 2022-01-27T20:19:07Z

src/results-output/results-doc.ts

+editQueue.onQueued(() => {
+  setTimeout(async () => {
+    while (editQueue.size() > 0) {
+      try {
+        await handleNextOutputQueueBatch();
+      } catch (err) {
+        console.error("Error writing output: ", err);
+      }
+    }
+  }, 100);
+});


There's some room for improvement on how the queue gets flushed but this gets the job done.

PEZ · 2022-01-27T20:23:20Z

Thanks for doing this!

stefan-toubia · 2022-01-27T20:46:36Z

I'm just happy to have finally identified the cause of this issue, it's been an almost daily issue for me for the past year or so!

bpringe · 2022-02-25T04:52:14Z

I also want to say thanks for spending time on this!

bpringe · 2022-02-26T23:34:03Z

src/results-output/results-doc.ts

+  onAppended?: OnAppendedCallback;
+};
+type QueuedCallback = (item: EditQueueItem) => void;
+export class EditQueue {


Would you mind refactoring this code to be more functional (similar to how it was, but with the new functionality you've applied)? I'd prefer we not use a class for this. I think classes tend to increase complexity and increase the likelihood of bugs and/or hard to maintain code.

I could help with this if you want.

Actually, maybe hold off on that. I left a comment on the issue about a repro case: #1509 (comment). I want to make sure we understand the cause better before we make changes.

stefan-toubia · 2022-03-02T16:49:53Z

I'll be away for the next week or so, but I'll revisit this when I'm back.

stefan-toubia · 2022-03-28T00:02:10Z

@PEZ @bpringe

I revisited this and cleaned it up for review. I kept the original batching logic for the most part, but cleaned up the append function by breaking it down into separate parts. The most important change here that will fix the issue of the output getting into a bad state is the addition of the try/catch to protect the outputPending flag.

I'm also pretty sure I was able to figure out how to reproduce this issue, described here #1641.

PEZ

Thanks for doing this! ❤️

The changes look good to me. I have tested the branch and things behave as I expect.

Please update the PR protocol adding a changelog entry, etc, and we're good to go, imo. 🙏

stefan-toubia · 2022-03-28T18:15:07Z

@PEZ I've updated the changelog and tested the vsix that I built locally, but I was unsure how to use the artifact from ci/circleci: build. This downloaded as a zip file that extracts into a directory, I'm not sure how to convert that into a vsix file.

PEZ · 2022-03-28T19:25:51Z

CHANGELOG.md

 ## [Unreleased]

+- Fix: [Results doc gets in a bad state and does not update](https://github.com/BetterThanTomorrow/calva/issues/1509)
+


Because reasons, we can't have an empty row between the header and the list

Suggested change

## [Unreleased]

- Fix: [Results doc gets in a bad state and does not update](https://github.com/BetterThanTomorrow/calva/issues/1509)

## [Unreleased]

- Fix: [Results doc gets in a bad state and does not update](https://github.com/BetterThanTomorrow/calva/issues/1509)

PEZ

Thanks again. This is good to be merged now. I can take care of the changelog thing after merge.

PEZ · 2022-03-28T19:29:42Z

@stefan-toubia we should update the PR template about that testing the VSIX is a bit optional and more necessary when changing in the CLJS code. As for the .zip file ending. Sometimes it downloads like that, I have no clue why. But a .vsix file is really just a zip-file with some special content in it. so changing the file extension is enough.

PEZ · 2022-03-28T19:31:15Z

🎉 ❤️ 🙏

bpringe

Good stuff!

stefan-toubia commented Jan 27, 2022

View reviewed changes

bpringe requested changes Feb 26, 2022

View reviewed changes

Refactor results doc output buffering

c5d6106

stefan-toubia force-pushed the improved-output-queuing branch from 81947b6 to c5d6106 Compare March 27, 2022 23:47

stefan-toubia mentioned this pull request Mar 28, 2022

Output can be written to the results doc before it has been initialized #1641

Open

Fix trailing newline

b11d04c

stefan-toubia requested a review from bpringe March 28, 2022 00:28

stefan-toubia marked this pull request as ready for review March 28, 2022 00:28

stefan-toubia added 2 commits March 27, 2022 17:31

Cleanup

ba736bc

Log error

dcb4bfc

PEZ reviewed Mar 28, 2022

View reviewed changes

Fixup

b5f35ec

stefan-toubia requested a review from PEZ March 28, 2022 18:15

PEZ reviewed Mar 28, 2022

View reviewed changes

PEZ approved these changes Mar 28, 2022

View reviewed changes

Merge branch 'dev' into improved-output-queuing

acf2a32

PEZ merged commit d0d24c8 into BetterThanTomorrow:dev Mar 28, 2022

bpringe reviewed Mar 29, 2022

View reviewed changes

stefan-toubia deleted the improved-output-queuing branch March 30, 2022 01:57

bpringe mentioned this pull request Apr 4, 2022

REPL stuck and requires window reload occasionally #1189

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update output queueing and batching logic #1510

Update output queueing and batching logic #1510

stefan-toubia commented Jan 27, 2022 •

edited

Loading

stefan-toubia Jan 27, 2022

PEZ Jan 27, 2022

bpringe Jan 31, 2022

stefan-toubia Feb 23, 2022

bpringe Feb 25, 2022 •

edited

Loading

stefan-toubia Jan 27, 2022

PEZ commented Jan 27, 2022

stefan-toubia commented Jan 27, 2022

bpringe commented Feb 25, 2022

bpringe Feb 26, 2022

bpringe Feb 27, 2022

stefan-toubia commented Mar 2, 2022

stefan-toubia commented Mar 28, 2022

PEZ left a comment

stefan-toubia commented Mar 28, 2022

PEZ Mar 28, 2022

PEZ left a comment

PEZ commented Mar 28, 2022

PEZ commented Mar 28, 2022

bpringe left a comment

		## [Unreleased]

		- Fix: [Results doc gets in a bad state and does not update](https://github.com/BetterThanTomorrow/calva/issues/1509)

Update output queueing and batching logic #1510

Update output queueing and batching logic #1510

Conversation

stefan-toubia commented Jan 27, 2022 • edited Loading

What has Changed?

My Calva PR Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bpringe Feb 25, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PEZ commented Jan 27, 2022

stefan-toubia commented Jan 27, 2022

bpringe commented Feb 25, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stefan-toubia commented Mar 2, 2022

stefan-toubia commented Mar 28, 2022

PEZ left a comment

Choose a reason for hiding this comment

stefan-toubia commented Mar 28, 2022

Choose a reason for hiding this comment

PEZ left a comment

Choose a reason for hiding this comment

PEZ commented Mar 28, 2022

PEZ commented Mar 28, 2022

bpringe left a comment

Choose a reason for hiding this comment

stefan-toubia commented Jan 27, 2022 •

edited

Loading

bpringe Feb 25, 2022 •

edited

Loading