teodesian / playwright-perl

Perl bindings for playwright
MIT License
22 stars 4 forks source link

Shutdown of playwright_server failing during global destruction #64

Open kcaran opened 2 months ago

kcaran commented 2 months ago

When I run tests on my macbook (perl 5.36), I get the following error at the end of the program:

(in cleanup) Can't call method "request" on an undefined value at /usr/local/lib/perl5/site_perl/5.36.1/Playwright/Util.pm line 39 during global destruction.

I've confirmed that the LWP::UserAgent object used by the Playwright handle is being destroyed before the Playwright object itself.

Creating a new LWP::UserAgent object inside of Playwright::quit() before (or inside) the call to Playwright::Util::request() fixes the issue, but I'm not sure that is the cleanest way to solve it. But it doesn't look like there is an easy way to guarantee object destruction order.

teodesian commented 2 months ago

There isn't a way to guarantee order during global destruction, unfortunately. When you have objects that have other objects as properties you rely on during teardown, this can be a problem.

I thought I caught and warned about this, but maybe not. Anyways, it's another reason I built the reaper.

I'll look at this and your other PR this weekend. Thanks Keith.

teodesian commented 2 months ago

I think the best way to handle this is via some kind of memoizer function to grab the LWP object (so I can worst case just make a new one) to reach out and murder playwright server processes. I'll do that.

kcaran commented 2 months ago

I was going to suggest recreating the UserAgent object if it was destroyed. The UserAgent object doesn't seem complicated to me to enough to warrant a more thorough solution.

243c243
<         $self->{ua} // LWP::UserAgent->new() );
---
>         $self->{ua} );
teodesian commented 2 months ago

The reason I allow passing of the UA is not just for testing purposes. Sometimes people need to do things like setup proxy configurations and so forth which I will have insufficient information available to be capable of reproducing.

I think to satisfy that need, I should allow the user to provide a subroutine which produces a UA as this argument. That sounds like a good feature request -- I'll reopen along those lines.

teodesian commented 2 months ago

Ideally the ua parameter should also accept a UA object to preserve back-compat.