GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — Add `Dispose` member to `MailboxProcessor` by bmitc · Pull Request #14929 · dotnet/fsharp

Conversation

@bmitc

@bmitc bmitc commented Mar 18, 2023

Copy link
Copy Markdown
Contributor

Closes fsharp/fslang-suggestions#1198

This rather minimal change adds a public Dispose member to the MailboxProcessor. The benefits are:

  • Better discoverability that MailboxProcessor implements IDisposable and thus must be disposed
  • Removes need to manually cast to IDisposable, as in (mailboxProcessor :> IDisposable).Dispose() before disposing

Please see the above linked suggestion for more details.

Some questions:

  1. Is the documentation I added sufficient?
  2. Are there examples I should be updating? I'm not sure where or how to find them.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@bmitc
bmitc requested a review from a team as a code owner March 18, 2023 20:34
@bmitc
Add Dispose member to MailboxProcessor
bbf35e6
@bmitc
bmitc force-pushed the main branch from 4cb6246 to bbf35e6 Compare March 19, 2023 00:57
@bmitc
Update FSharp.Core.SurfaceArea.netstandard21.release.bsl
bd91ecb
@bmitc

bmitc commented Mar 19, 2023

Copy link
Copy Markdown
Contributor Author

I am unsure how to solve the failing tests. Here's the error:

  Failed FSharp.Core.UnitTests.Portable.SurfaceArea.SurfaceAreaTest.VerifySurfaceAreaFSharpCore [239 ms]
  Error Message:
   System.Exception : Assembly: FSharp.Core, Version=7.0.0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a

              Expected and actual surface area don't match. To see the delta, run:
                  windiff D:\a\_work\1\s\tests\FSharp.Core.UnitTests\FSharp.Core.SurfaceArea.netstandard20.release.bsl D:\a\_work\1\s\artifacts\bin\FSharp.Core.UnitTests\Release\net472\FSharp.Core.SurfaceArea.netstandard20.release.out

              Unexpectedly missing (expected, not actual):

Unexpectedly present (actual, not expected):
    Microsoft.FSharp.Control.FSharpMailboxProcessor`1[TMsg]: Void Dispose()

Previously, the error message was "Unexpectedly present", and so I edited the [FSharp.Core.SurfaceArea.netstandard21.release.bsl](https://github.com/dotnet/fsharp/pull/14929/commits/bd91ecb01262c9c434185612afad8e21b2e26e51) to include the MailboxProcessor's new Dispose method. (This was a guess after trying to debug this issue locally.)

Could someone help me out here and explain what the surface area test is doing and how I should go about fixing it? It would be much appreciated. Thank you!

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@smoothdeveloper

smoothdeveloper commented Mar 19, 2023

Copy link
Copy Markdown
Contributor

@bmitc, if you copy the .out over the .bsl that are pointed by the test output, you should be able to see the diff when you are about to commit the difference.

bmitc reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@bmitc
Update FSharp.Core.SurfaceArea.netstandard20.release.bsl
34340b7
@bmitc

bmitc commented Mar 19, 2023

Copy link
Copy Markdown
Contributor Author

Thank you for the confirmation that that is the appropriate thing to do! My previous commit was just a guess toward that end, but I wasn't for sure if something was generating the BSL file, and just copying over the actual output over the expected felt like "cheating" the test. Haha.

What does BSL stand for? Is it just there to keep track of the available types, functions, members, etc. that are exposed by the DLLs and make sure the DLLs expose these as expected with new builds?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@psfinaki psfinaki mentioned this pull request Mar 20, 2023
Minor clarifications to baseline test docs #14934
Merged
@psfinaki

psfinaki commented Mar 20, 2023

Copy link
Copy Markdown
Contributor

@bmitc BSL means baseline. We have some obscure acronyms in the repo - should have been BSLN, right? :) Added a PR to clarify this a bit.

Otherwise, your understanding is correct. You can read a bit more in the TESTGUIDE.

And thanks for the contribution!

bmitc reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

psfinaki
psfinaki approved these changes Mar 20, 2023
View reviewed changes
abonie
abonie approved these changes Mar 20, 2023
View reviewed changes
@abonie
abonie merged commit 3c5e338 into dotnet:main Mar 20, 2023
@bmitc

bmitc commented Mar 20, 2023

Copy link
Copy Markdown
Contributor Author

Thank you for the clarification @psfinaki! I had later read about the BSL here, but thanks for the updating the docs! Thanks for the help everyone and for merging.

Also, I was looking to add a unit test for Dispose. I noted that there is this test, but there are no assertions. I was thinking about testing for the ObjectDisposedException, but I am not entirely sure of the mechanics there. According to the docs, it should be thrown anytime an IDisposable has been disposed and a member is called on the object, but I can't get anything in F# to actually throw that exception.

Are there any ideas as to a proper test for Dispose? I suppose one approach is to just have a test that at least calls Dispose?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@bmitc bmitc mentioned this pull request Mar 20, 2023
Add Dispose method to MailboxProcessor fsharp/fslang-suggestions#1198
Closed
5 tasks done
@psfinaki

psfinaki commented Mar 21, 2023

Copy link
Copy Markdown
Contributor

@bmitc so AFAIU the test you link just checks that nothing blows up. I think xUnit now doesn't have things like Assert.DoesNotThrow hence the test looks how it looks.

Otherwise, I've found this thread about testing the Dispose method. I'm all up for testing in general, but if you think it's an overkill, that's fine.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@psfinaki psfinaki mentioned this pull request Mar 27, 2023
Add FSharpType.IsMeasureType #14978
Merged
@qwbarch qwbarch mentioned this pull request Jan 12, 2024
Add BlockingQueueAgent#Dispose fsprojects/FSharpx.Async#57
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@abonie abonie abonie approved these changes
+1 more reviewer
@psfinaki psfinaki psfinaki approved these changes
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

F# Compiler and Tooling
Archived in project

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Add Dispose method to MailboxProcessor

Uh oh!

There was an error while loading. Please reload this page.

4 participants

@bmitc @smoothdeveloper @psfinaki @abonie
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.