Shouldn't roots be disposed when their scope function throws?

[AFatNiBBa]

Describe the bug

When something throws (Directly) inside of a function passed to createRoot(), the root that was being created doesn't get disposed.
Is this expected?

Your Example Website or App

https://playground.solidjs.com/anonymous/1e6d765f-717a-4fef-a981-573593533dcc

Steps to Reproduce the Bug or Issue

1. Go to the playground link
2. Check the console

Expected behavior

The root should be disposed and "Disposing" should be logged

Screenshots or Videos

No response

Platform

(Irrelevant)

Additional context

No response

[AFatNiBBa]

During dev mode, the component could be automatically disposed due to solid-refresh, in that case you need to put

/* @refresh skip */

at the start of your file to reproduce the issue

[ryansolid]

It's an interesting question.. Usually it is the parent or some manual mechanism responsible for disposal. That being said if you don't even get access to the dispose then what can anyone expect to do with it. Honestly it never occurred to me to dispose in this case. Top level render dispose call also destroys the DOM nodes. Is that expected? Hmm.. I might need to think about this.

[AFatNiBBa]

I think it's probably expected that whatever render() did will get reverted by the disposal, even the DOM nodes

[ryansolid]

Thinking about this more, I agree this is worth addressing in 2.0.

Even if the current 1.x zero-argument createRoot form is treated as unowned, if root setup throws before the caller can receive a disposer, there is no reasonable way for user code to clean up anything registered during that setup. In that case Solid should dispose the partially-created root before rethrowing.

I don’t want to change 1.x ownership semantics this late, but for 2.0 I think the invariant should be: if creating a root/render scope throws before returning control to the caller, Solid cleans up the partially-created scope.

[mizulu]

@ryansolid
similar for createEffect. https://s.olid.uk/id/ignl4J7zTaOBatiR9aIShQ

There is also the question of if running cleanups on a throw(which puts us in an unexpected state)
will be correct ( I have not considered the context of a render)

for example if I increment a value, and the cleanup should decrement it
but the throw was before the value was incremented. it will result in a discrepancy

in createRoot case it will be good if we can maybe provide the user an on error callback with a dispose
so they can decide.

maybe something like

function createRoot<T>(
  fn: (dispose: () => void) => T,
  error: (err, dispose: () => void) => T
  detachedOwner?: Owner
): T;

[ryansolid]

In 2.0 there is no cleanup on the effect side until the function runs to completion. I don't think we need a user callback here. The question is around reactive cleanup (onCleanup) which mid tree actually would be unsafe with concurrency etc.. But in non-reactive scope like an owner it is more interesting. That being said if we aren't capturing the errors in this scope then maybe this also doesn't make sense to do. Like its the parent that should be responsible. Especialy now that roots are owned. The only really questionable one is top level render etc... maybe this is just a try catch around the whole sync render function and call it a day.

[AFatNiBBa]

How would a try-catch AROUND the render call solve the issue?
Things would still not get disposed

[mizulu]

actually rethinking this

I don't think createRoot should auto dispose when something throw in it

the root has access to the dispose, and it has access to try..catch

if they want to dispose on exception they simply wrap it in try..catch

at least for createRoot I think it should be up to the user.

[ryansolid]

How would a try-catch AROUND the render call solve the issue? Things would still not get disposed

I mean in the catch block of render call dispose. Like internally in the render functino. The thing is without know you are topmost I don't think it makes sense todo. You can createRoot inside a memo etc. ErrorBoundaries handle more sophisticated cases like error through the UI tree. This is pure I ran the function and it failed scenarios.