puppeteer-sharp icon indicating copy to clipboard operation
puppeteer-sharp copied to clipboard

Fixes bug #1016: Unhandled TargetClosedException

Open R2D221 opened this issue 4 years ago • 13 comments

It solves the issue by collecting the lingering tasks, and manually making them throw so that the UnobservedTaskException event doesn't see them anymore.

R2D221 avatar Jul 16 '19 17:07 R2D221

I tried two different methods (you can see there are two commits), but it's weird, because the tests run fine in my machine for either of them, but AppVeyor doesn't seem happy about them.

It may be a little harder than I thought.

R2D221 avatar Jul 16 '19 21:07 R2D221

@R2D221 yes, not easy. But AppVeyor says which code is good and which isn't :p

If you want to keep trying stuff and pushing code, go for it, I have no problem with that.

kblok avatar Jul 16 '19 21:07 kblok

What do you think about adding these 2 lines in this PR? https://github.com/kblok/puppeteer-sharp/pull/1152/files#diff-408613782162a72f6aace17be118570aR36

If we want to guarantee that we are observing all exceptions we would need to test it across the board.

kblok avatar Jul 17 '19 11:07 kblok

I think it makes sense, but if you throw an exception in the unobserved event, I'm not exactly sure where it will throw it.

The approach I was using is collecting the errors in a list, forcing GC to collect, and then see if the list is empty. I don't know if this can be set up globally for all tests.

R2D221 avatar Jul 17 '19 15:07 R2D221

That's true. In my PR I added a Collect as you did. Maybe we could add that as a safety net when you finish this feature? We should never get that. Or at list a Console.WriteLine to get that in CI?

kblok avatar Jul 17 '19 15:07 kblok

Sure. Let me see how this run goes, and maybe we can add it as well.

R2D221 avatar Jul 17 '19 15:07 R2D221

One more thing. Could you test this on BrowserDisconnectTests.ShouldRejectNavigationWhenBrowserCloses? That was my headache on #1152

kblok avatar Jul 19 '19 11:07 kblok

I was busy on the weekend, but I tried to take a look at that.

I saw you created a variable LazyBodyLoadedTaskWrapper, but I think that's not the best way to solve it. Instead of deferring the creation of the task, we should move the exception logic outside of it. Let the task resolve with True or False as a success flag, and only throw an exception when it resolved as False (in this case, when we try to get the response body of a redirect).

I'll push another commit with the way I'm solving it. Just give me a moment.

(The last commit doesn't have a test associated. I'm kind of busy at work, but you get the idea)

R2D221 avatar Jul 22 '19 20:07 R2D221

In general, I think the way to go is checking how the tasks are failing unobserved, and think around it and only fail after we observe them.

R2D221 avatar Jul 22 '19 20:07 R2D221

Looking good, looking good. Thanks for your hard work!

kblok avatar Jul 22 '19 20:07 kblok

Hey there @kblok @R2D221 !

Do you have any status for the PR ? It seems that I have a problem and these changes could fix it. Described at issue #1801 :smile:

Cheers!

HunteRoi avatar Sep 09 '21 08:09 HunteRoi

@HunteRoi Hi!

Well, I was working on this a while ago, but if I recall correctly, I couldn't completely fix it. So I'd have to see what's missing.

R2D221 avatar Sep 09 '21 21:09 R2D221

Keep us in the loop please :smile:

HunteRoi avatar Sep 23 '21 07:09 HunteRoi