googleapis / google-api-dotnet-client

Google APIs Client Library for .NET
https://developers.google.com/api-client-library/dotnet
Apache License 2.0
1.33k stars 523 forks source link

GetRequest.DownloadAsync crash on Release Config #752

Closed mateusz-piasecki closed 7 years ago

mateusz-piasecki commented 8 years ago

I have Windows 10 store app. After updating to v1.13 of Google.Apis.Drive.v3 executing GetRequest.DownloadAsync throws: Exception thrown: 'System.PlatformNotSupportedException' in Microsoft.Threading.Tasks.Extensions.dll Exception thrown: 'System.PlatformNotSupportedException' in Microsoft.Threading.Tasks.dll

but (It's important) ONLY IN RELEASE CONFIG. In DEBUG works fine.

jskeet commented 8 years ago

Which version were you on before? (Just so we can see where the differences come from.)

mateusz-piasecki commented 8 years ago

Last version where it works is 1.12.0.455

jskeet commented 8 years ago

So is this using MediaDownloader? It doesn't sound like it, but there were very few changes between 1.12 and 1.13, and the one for MediaDownloader is the only one which sounds like it could be relevant here. So it definitely worked even in release configuration in 1.12? Is there any more information in the stack trace? This is very surprising :(

mateusz-piasecki commented 8 years ago

Yes, it is definitely working in 1.12 because I have store app which is using this version. I've updated to 1.13 and had to downgrade fast, because after update app stopped working (there was error or download attempt on any file). I was very suprised, because I'm tesing it each time before submitting an update. This time was no diffrent, but error was there anyway. So I checked in RELEASE config and find this^^. For the time beeing I've returned to v1.12 (same my-code) and all works fine.

jskeet commented 8 years ago

Okay - and presumably this happens consistently? I should presumably be able to just create a Universal Windows App, make a GetRequest.DownloadAsync and see the problem? Will give it a try - but I'm afraid it won't be for a week as I'm on vacation next week. In the meantime I'll assign it to Chris who may be able to help sooner.

Just thinking about the different versions, I've just remembered that actually we need to be thinking about the differences between 1.11 and 1.13, as there wasn't a 1.12 in the support libraries.

mateusz-piasecki commented 8 years ago

You're right I downgraded support libraries to 1.11. I created sample for you which recreates the problem. It is minimum of minimum of minimum, but it does what it should. To get it to work add your client_secret to Assets folder and complete proper file id (if you want to actually open the file, give proper file extension and save stream to StorageFile - it's not required to see error) PS. Sorry for closing - missclick GoogleDownload.zip

jskeet commented 8 years ago

Thanks so much for provide a repro - that'll make it much easier to help you. I may find time to have a look this weekend, but hopefully if not, Chris can reproduce it next week. We'll get there one way or another :)

chrisdunelm commented 8 years ago

Unfortunately I don't seem to be able to run the repro. I only have access to Windows on a VM (Windows Server 2012), which doesn't seem to like running the Windows Phone emulator, which I appear to need. I'm not very familiar with Universal Windows Apps, so if there's another way I can try this please let me know.

mateusz-piasecki commented 8 years ago

You can't use Windows Phone emulator - it's Windows 10 UWP app, so you'll need Windows 10 machine or Windows 10 Mobile emulator (which runs only on Windows 10 machine).

chrisdunelm commented 8 years ago

OK, thanks, I'll see if I can find a Windows 10 machine.

chrisdunelm commented 8 years ago

OK, I can reproduce this (with one difference). Using a Win10 VM.

Using the code to posted earlier, it works in debug mode with all versions of Google.Apis.Drive.v3 In release mode, it fails on v1.13 (as you found): 1.12.455 - works 1.12.480 - works 1.13.0.498 - fails 1.13.1.505 - fails

The difference is that I don't appear to get an exception thrown when it fails, it just puts no data into the resposne stream.

I'll see if I can find what's causing this...

mateusz-piasecki commented 8 years ago

Exception isn't thrown to 'the surface', but you can see it in Output window in VS. Besides this, I agree with all.

chrisdunelm commented 8 years ago

Ah yes, I see them, thanks.

chrisdunelm commented 8 years ago

The problem occurs when .NET native compilation is enabled, which by default is in release builds, but not in debug builds.

chrsmith commented 8 years ago

@MattWhilden works at Microsoft and is an expert in all things UMP. Perhaps he can provide some tips for how to troubleshoot this.

chrisdunelm commented 8 years ago

The problem occurs in the ReadAsync method in MediaDownloader.cs I've recreated the problem in a much smaller testcase. It's something to do with that ReadAsync call being in a portable library. The problem does not occur if the ReadAsync call is in the UWP app itself.

MattWhilden commented 8 years ago

Hello! I work on the .NET Native runtime and compiler team and would be happy to take a look at this. At the risk of admitting my complete ignorance publicly, I seem to have hit a wall generating the correct set of client secrets from my fresh new google cloud account. @chrsmith was going to help sort me out this afternoon but if someone has any tips I'd be grateful.

jskeet commented 8 years ago

@MattWhilden: If you run gcloud auth login from the Cloud SDK that should populate the default credentials. Or you could generate a JSON file with service credentials and set the GOOGLE_APPLICATION_CREDENTIALS environment variable to point to that file.

(Personally I find the SDK approach simpler, but the service account approach means you don't need to install anything.)

chrisdunelm commented 8 years ago

@MattWhilden Attached is a sln which reproduces this problem in a simpler way without any Google dependencies. The problem only occurs when native compilation is enabled, in Class1.cs (classic naming ;) of TestUwpLib on line 21. And is the same PlatformNotSupportedException. If the Class1.DoIt() method (another classic name) is moved to the TestUwp app, then it works as expected. My system is a Win10 VM, VS community 2015 version 14.0.25123.00 update 2, .NET 4.6.01038 Attached sln: TestUwp.zip Thanks!

MattWhilden commented 8 years ago

Thanks @jskeet and @chrisdunelm. I'm playing with the non googly repro now and will report back.

MattWhilden commented 8 years ago

Hmm. So if you have a PCL that contains platforms that didn't have Tasks support out of the box then you get built against a extensions dll that fills in the surface area. This library relies on some APIs that have been move elsewhere for UWP and the old APIs throw as you see here. We have a phase of the compiler that is supposed to do remapping from these "old style" portable APIs to the "new" ones that are available for UWP applications. I'm investigating why that doesn't seem to have kicked in and, presumably, will have a decent idea for a workaround at that time. Stay tuned.

MattWhilden commented 8 years ago

Okay so I've hit paydirt on this issue and it turns out that Microsoft.Threading.Tasks.Extensions has a chunk of code that currently has no way to run in UWP. How to best work around/fix the issue is still an open question but I figured I would report back about what triggers the problem and some thoughts about what may be decent workarounds.

The problem As above, we try to be helpful and remap things that have moved in the .NET surface area compared to the older portable API set. As an example Type no longer has GetType, that now lives on TypeInfo. Normally what would happen is that they would be redirected at runtime using Type Forwards. However, for .NET Native we try to resolve all of these ahead of time.

We fail 4 of these mappings inside on Microsoft.Threading.Tasks.Extensions but why. This library attempts to create a delegate of some of these mapped types and because delegate creation ends up needing the IL instruction ldvirtftn (instead of the more familiar callvirt) which the .NET Native compiler seems to explicitly disallow. Presumably this is because we were worried about not being able to correctly redirect this call in the face of overloads but I'm still chasing that a bit.

The what to do about it I've filed a bug against the .NET Native compiler so that we can investigate doing something better in cases like this. We can almost certainly do better than "just give up when we see ldvirtfn". Either proving that there are no overloads and doing the mapping or creating a runtime check or or or. We'll have to investigate that a bit. That fix (if we can do something that isn't a terrible hack) could be available before end of year.

I'm going to reach out to the folks here that own Microsoft.Threading.Tasks.Extensions and see if we can hack around the issue in that library. It may be worth seeing how quickly that could happen but it's on the order of a few weeks at the earliest I suspect. I will report back here when I know more, probably a day or two.

As a "workaround" that is entirely in your own control: Publishing a version of your library that avoids Microsoft.Threading.Tasks.Extensions may be somewhat palatable. As much as I hate to say it, it seems as though Silverlight 5 being part of the portable targets is the only thing causing the problematic library to be necessary.

Happy to answer other questions if you have them. I do appreciate the report and great repro case.


Side note: the warnings for builds with .NET Native turned on will try to alert you to our failings. Here's what it looks like on my machine. 1-4 are the compiler complaining about exactly the issue above. 5 and 6 are similar but come from clicking the checkbox in the build properties that is "enable static analysis for .NET Native". This allows some tooling to run that may kick out helpful warnings. In this case, it's noticing that those calls are only going to throw at runtime. 8 and 9 are spurious and can be safely ignored. warnings

chrisdunelm commented 8 years ago

@MattWhilden thanks for the really helpful explanation. I confirm that removing silverlight 5 from the library targets does fix the simple test case. I'll now see how this relates back to the original problem.

mateusz-piasecki commented 8 years ago

@chrisdunelm any progress on this matter?

chrisdunelm commented 8 years ago

@Piachu91 Apologies, nothing yet. We will fix this, but have had other priorities over the last couple of weeks. I hope to get back to this next week.

chrisdunelm commented 8 years ago

We're working on additionally targeting NetStandard (<= 1.3, not exactly sure yet) for this library, which as I understand it should fix this on UWP. @MattWhilden If I'm wrong about that, please let me know ;) No firm date for when this will be done but hopefully fairly soon, if no large blockers arise.

MattWhilden commented 8 years ago

That should correct the issue. If you have any trouble with that migration, we have folks available to answer questions at port2core@microsoft.com. Feedback about API differences or missing APIs are greatly welcomed.

chrisdunelm commented 8 years ago

Great, thanks. @MattWhilden If we provide a nupkg that supports NetStandard1.3, Profile259 and Profile328; which target will a UWP project use? Is there a well-defined precedence?

ericstj commented 8 years ago

I believe netstandard1.3. NuGet docs are terse on this and just link to the source code, which of course is not helpful. This must be a compatibility contract that cannot change version to version so I would expect NuGet to document the full precedence expansion of their algorithm. /cc @yishaigalatzer @robrelyea

jskeet commented 8 years ago

@MattWhilden I've just sent a separate mail (about porting packages containing native libraries) to port2core@microsoft.com, and it bounced as I'm not in the approved senders list. What's the best route for getting advice here?

MattWhilden commented 8 years ago

I actually see your mail in so I'll have to hunt down why you're seeing a bounce. Thanks for letting me know.

yishaigalatzer commented 8 years ago

Consider sticking with netstandard only if possible, I'm not sure if it is relevant and if you need the pcls for another reason

chrisdunelm commented 8 years ago

@yishaigalatzer thanks for you comment. Unfortunately we currently need the PCLs as we need to continue support for the platforms that don't support netstandard1.3 (net<4.6; Windows 8.0 & 8.1; Windows phone; and Windows phone silverlight).

Is there a well-defined behavior for platforms that have support for multiple targeted platforms within a package?

harikmenon commented 8 years ago

you are in luck @chrisdunelm, one of our awesome nuget engineers @joelverhagen decided to do something about this and built this site over the weekend that will help you understand which is the nearest framework that would get selected based on the project TFM and the list of package frameworks. Do try this out and let us know if this is what you are looking for. While we don't have explicit documentation that calls out the precedence rules, this site should help you better understand them.

http://nugettoolsdev.azurewebsites.net/3.5.0-beta2/get-nearest-framework?project=net462&package=net20%0D%0Anetstandard1.0%0D%0Aportable-net45%2Bwin8%0D%0A

harikmenon commented 8 years ago

and yes will look into adding this to our docs. https://github.com/NuGet/NuGetDocs/issues/495

chrisdunelm commented 8 years ago

@harikmenon @joelverhagen - thanks :) That is really useful. Only comment is if the "Package Frameworks" entry box could be made a bit wider. Some of the portable-* names are too long to fit on a single line. E.g. "portable-net40+sl50+netcore45+wpa81+wp8"

Coming back to the original bug, we hope to soon be releasing a version of the library that targets:

This confirms that a UWP app (uap target) will select the netstandard1.3 framework given these choices, which is exactly what I was hoping :)

harikmenon commented 8 years ago

Sweetness!!! Glad it helped. Do reach out if you need anything else :)

mateusz-piasecki commented 8 years ago

@chrisdunelm any progress? Maybe some timeline for fix?

chrisdunelm commented 8 years ago

This is proving more problematic than anticipated. The nupkg needs to contain a win81 target (which will also be used in uap10.0 apps), which we have not yet managed to get working with a UWP release build - it always crashes with unhelpful error messages. It's possible there's a static initialization problem, which we're going to be investigating further this week. No timeline I'm afraid, but we are still working on it.

harikmenon commented 8 years ago

The UWP release build might be due to .NET Native. You can reach out to .NET Native (External Feedback) dotnetnative@microsoft.com If you need some help.

harikmenon commented 8 years ago

Since this is a library and not an app, you don't have to build NET Native output. you can ship IL and the consuming app will convert the resulting app to .NET native. However, you need to test apps that are release and consuming your binaries though to make sure it works in .NET Native.

chrisdunelm commented 8 years ago

@harikmenon thanks. Yes, our library is shipped as IL in the nupkg, then we have a Win10 test app which consumes that library and crashes in a release build. Works OK in a debug build. We hope to be investigating more this week, and will update here and contact dotnetnative@ as appropriate :)

Quick question - in VS 2015 update 2 (community) there was a project build option "Compile with .NET native tool chain". This has gone in update 3, and I can't find an equivalent. Is this expected? And how is native compilation now enabled/disabled?

harikmenon commented 8 years ago

Hmm, do you mind reaching out the alias I had sent you before? They would know the answer for sure for all things .NEt Native :)

MattWhilden commented 8 years ago

@chrisdunelm please do reach out to us. "Works on DEBUG but not on RELEASE" is exactly the symptom for issues we are fixing with .NET Native.

As for the checkbox... That means that VS doesn't think you've installed our tools onto that machine. We should be getting installed with the UWP tools. Perhaps running a repair for that SDK will get you back into shape?

abrar84 commented 8 years ago

Hi everybody, can anyone of you please me regarding speech API? I am trying to analyze an audio of 45 second using this .NET API but getting transcript of only first 5 seconds. Sorry if asked this on wrong forum.

Thanks

MattWhilden commented 8 years ago

@abrar84 This won't be the right place to get help for that but if you mail either Hari or I with details (we both work at MS), we can probably get your question routed to the right place. harikm@THENAMEOFOURCOMPANY.com, mwhilden@THENAMEOFOURCOMPANY.com.

chrisdunelm commented 8 years ago

@harikmenon, @MattWhilden - yes, I'll contact you at the alias you gave. I've been distracted away from this for a few days, but should be back on it in the next day or two. Sorry for the delay.

@abrar84 which (who's ;) speech api are you having problems with?

abrar84 commented 8 years ago

@chrisdunelm Its about https://developers.google.com/api-client-library/dotnet/apis/speech/v1beta1

chrisdunelm commented 8 years ago

@abrar84, thanks for creating issue #791 - we'll follow up there.

mateusz-piasecki commented 8 years ago

@chrisdunelm @MattWhilden any luck? It's not working since may, and I need new ResumableUpload functionality