Fix receiving and processing mailbox after Dispose

[rstm-sf]

Fixed #10720

Is it worth worrying about it?

fsharp/src/fsharp/FSharp.Core/mailbox.fs

Lines 106 to 110 in dbf9a62

	member _.inbox =
	match inboxStore with
	| null -> inboxStore <- new System.Collections.Generic.List<'Msg>(1)
	| _ -> ()
	inboxStore

[rstm-sf]

I rewrote async to task and got a flaky test? https://dev.azure.com/dnceng/public/_build/results?buildId=1735500&view=logs&j=7bab896a-24f8-544f-51eb-43745367a332&t=999dbed9-85e3-59ab-57f0-3e22828b5bad&l=4813

[rstm-sf]

Sorry I don't understand this error https://dev.azure.com/dnceng-public/public/_build/results?buildId=392941&view=logs&j=882ba401-d5d6-57d5-a958-980a439dbeb8&t=03421b2c-e47d-52ac-cb71-f3db07decb69&l=3480

Failed FSharp.Editor.IntegrationTests.CodeActionTests.UnusedOpenDeclarations (VS2022) [5 s]
  Error Message:
   System.Threading.Tasks.TaskCanceledException : A task was canceled.
  Stack Trace:
   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
   at FSharp.Editor.IntegrationTests.Helpers.LightBulbHelper.<WaitForItemsAsync>d__0.MoveNext() in /_/vsintegration/tests/FSharp.Editor.IntegrationTests/Helpers/LightBulbHelper.cs:line 63
--- End of stack trace from previous location where exception was thrown ---
   at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
   at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task)
   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
   at Microsoft.VisualStudio.Extensibility.Testing.EditorInProcess.<InvokeCodeActionListAsync>d__4.MoveNext() in /_/vsintegration/tests/FSharp.Editor.IntegrationTests/InProcess/EditorInProcess.cs:line 91
--- End of stack trace from previous location where exception was thrown ---
   at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
   at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task)
   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
   at FSharp.Editor.IntegrationTests.CodeActionTests.<UnusedOpenDeclarations>d__0.MoveNext() in /_/vsintegration/tests/FSharp.Editor.IntegrationTests/CodeActionTests.cs:line 33
--- End of stack trace from previous location where exception was thrown ---
   at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
   at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task)
   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
   at System.Runtime.CompilerServices.TaskAwaiter.GetResult()
--- End of stack trace from previous location where exception was thrown ---
   at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
   at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task)
   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
--- End of stack trace from previous location where exception was thrown ---
   at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
   at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task)
   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
  Passed FSharp.Editor.IntegrationTests.CodeActionTests.AddMissingFunKeyword (VS2022) [2 s]
  Passed FSharp.Editor.IntegrationTests.CreateProjectTests.ConsoleApp (VS2022) [8 s]
  Passed FSharp.Editor.IntegrationTests.CreateProjectTests.ClassLibrary (VS2022) [536 ms]
  Passed FSharp.Editor.IntegrationTests.CreateProjectTests.XUnitTestProject (VS2022) [427 ms]
  Passed FSharp.Editor.IntegrationTests.GoToDefinitionTests.FsiAndFsFilesGoToCorrespondentDefinitions (VS2022) [19 s]
  Passed FSharp.Editor.IntegrationTests.GoToDefinitionTests.GoesToDefinition (VS2022) [3 s]

[psfinaki]

Sorry I don't understand this error

The error is unrelated, sorry for this, int tests fail once in a while. Looks like rebuild helped.

[psfinaki]

Thanks!

[0101]

This would be a breaking change. Maybe we should consider adding a new constructor with the new behavior.

[Lanayx]

This would be a breaking change

Do you mean somebody was relying on sending messages after mailbox disposal and that behaviour should be preserved?

[vzarytovskii]

This would be a breaking change

Do you mean somebody was relying on sending messages after mailbox disposal and that behaviour should be preserved?

Now it will throw on post if it's disposed.

[vzarytovskii]

@rstm-sf It's now a runtime breaking change (since new exception will be thrown at runtime, if Post is called on a disposed instance of the mailbox), I propose we either:

1. Don't throw exception at runtime, and just silently swallow it.
2. Add new Post overload method, which will accept an additional flag which will tell mailbox to throw.
3. Add new constructor, which will set flag for current mailbox, based on which it will either throw or not (with default value to not throw for backwards compatibility).

[github-actions]

✅ No release notes required

[rstm-sf]

@rstm-sf It's now a runtime breaking change (since new exception will be thrown at runtime, if Post is called on a disposed instance of the mailbox), I propose we either:

1. Don't throw exception at runtime, and just silently swallow it.

2. Add new `Post` overload method, which will accept an additional flag which will tell mailbox to throw.

3. Add new constructor, which will set flag for current mailbox, based on which it will either throw or not (with default value to **not** throw for backwards compatibility).

I've done all the steps, but 2 seems like a lot

[vzarytovskii]

@rstm-sf It's now a runtime breaking change (since new exception will be thrown at runtime, if Post is called on a disposed instance of the mailbox), I propose we either:

1. Don't throw exception at runtime, and just silently swallow it.

2. Add new `Post` overload method, which will accept an additional flag which will tell mailbox to throw.

3. Add new constructor, which will set flag for current mailbox, based on which it will either throw or not (with default value to **not** throw for backwards compatibility).

I've done all the steps, but 2 seems like a lot

Wait, no, I meant we do either for those three, not all. New constructor sounds like a good middle ground

[vzarytovskii]

Thanks