Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add another test for #3254 #3367

Merged
merged 2 commits into from
Jul 25, 2017
Merged

Conversation

matthid
Copy link
Contributor

@matthid matthid commented Jul 24, 2017

#3254 is a regression in FSharp.Core 4.2.1, this means paket needs a new release before we can update to the 4.2.X branch.

ported from paket. related to #3350 and fsprojects/Paket#2553 (same test is added there but it fails)

@msftclas
Copy link

@matthid,
Thanks for having already signed the Contribution License Agreement. Your agreement was validated by Microsoft. We will now review your pull request.
Thanks,
Microsoft Pull Request Bot

@matthid
Copy link
Contributor Author

matthid commented Jul 24, 2017

You can check that 4.2.1 is indeed faulty by checking out the rebased branch at https://github.com/matthid/visualfsharp/tree/cancellation_421 and running the test. It will be red.

@forki
Copy link
Contributor

forki commented Jul 24, 2017

@KevinRansom @dsyme can we get a 4.2.2 release with c6a070b applied? It's really unfortunate that the regression slipped through.

@forki
Copy link
Contributor

forki commented Jul 25, 2017

I really hope we find a fast solution here. The community gave up control over this package, but now MS needs to act fast. It looks like we have a FSharp.Core in the wild that doesn't call continuations properly. It's not just a Paket thing. There might be processes running that just hang now.

do! Async.Sleep 100
cts.Cancel()
do! Async.Sleep 100
tcs.TrySetException (TimeoutException "Cancellation was requested, but wasn't honered after 1 second. We finish the task forcefully (requests might still run in the background).")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"honored"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I changed the message completely in a way to make more sense in this context. Thanks!

@dsyme
Copy link
Contributor

dsyme commented Jul 25, 2017

@forki @matthid It is hard to tell what's what in the recent PRs for control.fs. Could you please make sure

  1. there is an issue filed for each problem you have identified, and that the issue has a complete problem description, a minimal repro, a list of workarounds etc.

  2. there is a link to the proposed fix for each issue, and that fix is minimal.

I'll keep looking over the issues and PRs to try to assess the impact of the changes you're proposing here

I really hope we find a fast solution here. The community gave up control over this package...

Please try to avoid rushing things, and let's follow our methodology here.

Also, please don't tie this into a community v. Microsoft narrative. The .NET Framework 4.x FSharp.Core.dll used by the FSharp.Core package has always been the latest signed FSharp.Core.dll produced by Microsoft - it has never so far been a community-recompiled one.

We have always as a community done that: when faced with a regression we wouldn't have done things any differently last month, or last year. We may indeed have shipped a regression, but we need to do full and careful analysis, with multiple code reviewers, as well as the actual fixing and testing you're doing, and we need to get the fix into a signed release and repackaged. We don't do micro-releases to FSharp.Core.dll as yet.

@dsyme
Copy link
Contributor

dsyme commented Jul 25, 2017

@KevinRansom I need to assess the other bugs, but I agree with @forki and @matthid that we minimally should push a new FSharp.Core package with the fix for #3254 included as soon as practical. That fix has been thoroughly reviewed and is important.

@forki
Copy link
Contributor

forki commented Jul 25, 2017

when faced with a regression we wouldn't have done things any differently last month, or last year

we might have pushed a new release or unlisted the old one.
But doesn't matter that much. Important is what we do now.

@vasily-kirichenko
Copy link
Contributor

I'm for unlisting 4.2.1 ASAP.

@forki
Copy link
Contributor

forki commented Jul 25, 2017

unlisting is not really an option because IIRC it's the first FSharp.Core that works for new SDK. So we would break that. Only option is forward

@dsyme
Copy link
Contributor

dsyme commented Jul 25, 2017

@matthid Thanks for the extra test for #3254, this is fantastic extra coverage

@dsyme dsyme merged commit 4559cfd into dotnet:master Jul 25, 2017
@matthid
Copy link
Contributor Author

matthid commented Jul 25, 2017

@dsyme Should we open another issue for FSharp.Core 4.2.1 being broken? I really feel like this is quite a frustrating bug to deal with. I'm not sure I could have detected it in paket if I wasn't the person who fixed it initially in FSharp.Core. And even then it took me quite a while to figure it out (the second time).

Please try to avoid rushing things, and let's follow our methodology here.

I agree, lets make sure the next release works. But we should consider unlisting 4.2.1 after we have that. @cartermp We might even need to add some release notes somewhere if we cannot deploy the fix anytime soon.

@dsyme
Copy link
Contributor

dsyme commented Jul 25, 2017

Should we open another issue for FSharp.Core 4.2.1 [containing a regression]

Yes, please do that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants