numberscope / frontscope

Numberscope's front end and user interface: responsible for specifying sequences and defining and displaying visualizers
MIT License
7 stars 14 forks source link

Is our handling of the nondeterminism of components mounting/unmounting appropriate? #295

Open gwhitney opened 2 months ago

gwhitney commented 2 months ago

MEA CULPA -- we don't have an explicit test that checks the result of npm run build, because we have no end-to-end tests, just the guideline in the review checklist that one should always try to run the result of the build. With the flurry of checkins lately, I neglected to always do that (npm run build; npm run preview). And now we have gotten into a state in which frontscope works fine in dev mode, but the built version interacts fine until you hit draw, and then there is a blank white area where the visualization should be, for any visualizer. Ugh ugh. I will try to debug. If someone wants to bisect to see where this happened, it could be helpful. I will be that it was the version bump to vue though. Not sure how we would back that up since that necessitated almost everything bumping along with it.

gwhitney commented 2 months ago

OK, it's a race condition: when you switch from the bundlecard to the main draw, the bundlecard tells the visualizer to dispose (because it has to get rid of the sketch that was on the bundlecard, because leaving sketches around leads to bad consequences). But the CanvasArea that is popping up also tells the visualizer to initialize (inhabit) a new div. In dev mode, those are happening in the desired order (dispose then inhabit). In production, they are happening in the bad order (inhabit then dispose, leaving nothing on screen and a dangling sketch). Currently no idea how to enforce the order, or whether the code just has to deal with the possibility of arbitrary ordering, but I will try to figure it out.

gwhitney commented 2 months ago

OK, Aaron and I discussed how to correct this: views of a visualizer are created by calls to inhabit, and they are destroyed by calls to dispose, which will be renamed to depart to make it more specifically opposite to inhabit. We will require that the div where the view should be is passed both to inhabit and depart. And we will (at least for now) enforce that there be at most one view of a visualizer by the following behaviors: inhabit: (a) if there is no view, creates one in the given div (b) if there is one in the given div, does nothing (c) if there in one in a different div, automatically depart()s that one for you, then creates a new one in the given div

depart: (a) if there is no view, throws an error (b) if there is one in the given div, removes it and stops its listening and releases its resources, etc. (c) if there is one in a different div, assumes that any there had been in the given div was already departed, and so silently does nothing.

That should get production running again. Will keep you all posted here.

Vectornaut commented 2 months ago

Documentation thought: a mnemonic to help developers to remember to call depart().

If a div gets removed from the DOM while a visualizer is still inhabiting it, the visualizer becomes a troublesome "ghost": the user can't see or interact with it, but it's still listening for events and consuming system resources. There are two ways to prevent this from happening.

  1. Immediately tell the visualizer to depart() the div you're removing.
  2. Immediately tell the visualizer to inhabit() a div that's still in the DOM.

It's safe for these calls to happen redundantly. If you tell a visualizer to depart() a div it's not inhabiting, or to inhabit() a div it's already inhabiting, nothing will happen.

Based on this narrative, I think calling depart() when there's no view should silently do nothing. I'd like developers to feel free to call inhabit() and depart() as needed, without worrying about getting errors. I can't think of any situation where I'd want to have certainty, enforced by runtime errors, that I'm never telling a visualizer to depart() when it's not inhabiting a div.

gwhitney commented 2 months ago

Based on this narrative, I think calling depart() when there's no view should silently do nothing. I'd like developers to feel free to call inhabit() and depart() as needed, without worrying about getting errors. I can't think of any situation where I'd want to have certainty, enforced by runtime errors, that I'm never telling a visualizer to depart() when it's not inhabiting a div.

Hmmm. The following history of all calls to inhabit/depart during a program is surely indicative of a logic error in that program:

depart(A)

So I think that should throw an error. The following history of calls will result in a depart when there is no view but is merely questionable, not necessarily any serious error (but could be the result of one):

inhabit(A); inhabit(B); depart(B); depart(A);

So I could see going two ways: (i) the current behavior, of throwing an error when departing if there is no view at all, is working OK in practice. So the conservative path is to leave it that way, and if the error comes up in a situation that we would like to be OK, deal with it then. (ii) Switch to throwing an error when departing only if that view has never inhabited anything (since that is surely an error), and otherwise silently do nothing on a depart when there is no view.

(i) seems safer: if we never get an error, fine; if we do, we look at what's going on and see if we want to allow that behavior, but at least we get the chance to look.

Vectornaut commented 2 months ago

The following history of all calls to inhabit/depart during a program is surely indicative of a logic error in that program...

Here's the sort of context I was imagining.

  1. Conditionally inhabit(A).
  2. depart(A) just in case it was inhabited before.

If you want to make sure a visualizer never departs a div it's not inhabiting, you'll have to explicitly remember whether the conditional got called. But I don't think you should have to do that, because the visualizer itself implicitly remembers all the information you need.

gwhitney commented 2 months ago

I don't know of any other resource acquisition/release protocol that works like this, dating way back to malloc/free in C. It was not valid to free a block of memory that you maybe malloced (but turns out you really didn't, but you just forgot that fact, oops). The closest analogue is that I think it was OK to free the null pointer, but there is no analogue of a null dom element. I don't think we want a design that facilitates release of resources you never actually acquired.

gwhitney commented 2 months ago

If we want to let clients use the visualizer itself as the storehouse of the info of whether it inhabited something, we could add a "residence()" method that returns the element it currently inhabits or undefined if none. My inclination would be to wait to add that method until someone wants it.

gwhitney commented 2 months ago

Reopening until we reach a consensus as to whether depart() on a visualizer not inhabiting any div should throw or be a no-op.