typester / anyevent-gearman-perl Goto Github PK

We have some 16 fixes by different people unmerged. I volunteer to merge them all into one big PR which can be easily merged.

However, I don't understand the rationale behind some of the fixes and their readiness to production. So I want the authors and @melo to express their opinions on what should be shipped in 0.11 and what should be postponed to 0.12

I propose the following 4 small changes for 0.11:

• fixed unregistering of functions in workers (patch by @kappa)
• fixed Makefile.PL to work in strict sub mode (patch by @gjtorikian)
• fixed on_fail not always firing (patch by @nponeccop)
• TravisCI support (patch by @nponeccop)

The first 2 are already on @melo branch, and 2 more are my patches (I obviously claim they are ready to prime time and important, lol).

Some changes are mere documentation changes:

• Add META.json (patch by @perlancar)
• improved build instructions (patch by @gjtorikian)
• typo fix (patch by @dsteinbrunner)

The @perlancar change I don't understand, and @gitorkian doc is misleading, as those changes are only needed when developing or installing from GitHub instead of CPAN.

Some changes are ongoing efforts to get rid of Any::Moose. I don't know if they are ready or not yet:

• Switch Any::Moose-basics to Moo by @melo
• Use Type::Tiny for types (patch by @hemmop)
• Use private writer for "rwp" attributes (patch by @hemmop)

The rest are some changes I don't fully understand (including 1 documentation change):

• Worker exception support (patch by @7ojo)
• use default gearman port by default (@plicease)
• Documented prefix on worker as public attribute (patch by @plicease)
• Begin adding support for priorities (patch by @iarna)
• branch of @saymedia (includes work by @athomason and @bbeeley)

So what do you think?

We need regenerate the folder (using a newer Module::Install etc) and reupload. This fixes the 2 warnings (precedence and sprint) our CPAN Testers demonstrate, such as

http://www.cpantesters.org/cpan/report/b174d204-fb92-11e6-9734-8509067225e0

Possible precedence issue with control flow operator at /tmpfs/.cpan-build-cpansand/2017022418/AnyEvent-Gearman-0.10-0/inc/Test/Builder.pm line 917.
Redundant argument in sprintf at /tmpfs/.cpan-build-cpansand/2017022418/AnyEvent-Gearman-0.10-0/inc/Spiffy.pm line 229.

https://metacpan.org/pod/CPAN::Meta::Spec#Phases has a rather rich language for specifying "extra" dependencies. And AnyEvent::Gearman has pretty many of these:

1. develop-requires dependencies (see #13 and gjtorikian@673c156 )

• Module::Install
• Module::Install::AuthorTests
• Module::Install::Repository
• Module::Install::TestBase

2. develop-recommends dependencies to run optional author tests:

• Test::Spelling
• Test::Pod::Coverage

2. test-recommends dependencies to run optional tests. Without them tests don't actually test anything except that it's not terribly broken:

• Gearman::Server
• Gearman::Worker

There appears to be a race condition or another heisenbug: Travis builds seldom fail there, I've seen 2 failures at this place with different Perl versions.

Should we disable the tests and/or drop support?

https://travis-ci.org/nponeccop/anyevent-gearman-perl/jobs/396206248 - see for logs

If you take a look at http://gearman.org/protocol/ you will find that there is a possibility to call few, administrative functions to gearmand.

They are: workers, status, maxqueue, shutdown and version.

workers - shows all worker addresses, fds, clientids and function they provide.
status - shows how many workers we have for each function and how many of them are busy.

maxqueue - with this one you could set max queue size for function

shutdown - shutdown the gearmand

version - shows gearmand version

The first two could can be handy (e.g.: to display currently available functions for clients, throwing an error if there is no such function, or just to get information about load of gearman "farm").

Module::Install is deprecated, and moreover we need more metadata for builds to work without manual configuration, see #14 and melo@673c156

So we may want to migrate to a different build system to make the metadata both more complete and more automatic.

I'll probably make feature branches using minilla and Dist-Zilla to see which is better. ShipIt seems abandoned and doesn't offer automation to the extent of Dist-Zilla.

I rebased hemmop/use_moo on top of nponeccop/all-fixes and got the following failures:

• Travis fails on 5.26 and 5.28 by hanging in t/02_client_worker.t, but works fine in earlier versions: https://travis-ci.org/nponeccop/anyevent-gearman-perl/builds/405493820
• Unlike Travis, AppVeyor script installs Test::Pod::Coverage so the coverage tests are not skipped. And they fail: https://ci.appveyor.com/project/nponeccop/anyevent-gearman-perl/build/job/lf5sfdttcgeqx0xe#L1588

@hemmop, @melo can you fix the tests?

The rebased branch is https://github.com/nponeccop/anyevent-gearman-perl/tree/use_moo

The client module leaks memory aggressively for me on Perl 5.14.0, Debian squeeze. For example, with a trivial client (https://gist.github.com/982072#file_client_anyevent.pl) and trivial worker (https://gist.github.com/982072#file_worker.pl), the client process rss sizes grows by 56,496kB after 10,000 jobs.

By comparison, a Gearman::Client::Async implementation of the same client does not leak (https://gist.github.com/982072#file_client_danga.pl).

The results are the same with the EV and Perl event backends. To test if AnyEvent itself was a problem, I also tested a mutant client that uses Gearman::Client::Async with AnyEvent instead of Danga::Socket (https://gist.github.com/982072#file_client_mutant.pl). It did not leak, suggesting an issue with AnyEvent::Gearman::Client.

Dumping the AnyEvent::Gearman::Client object with Data::Dumper after the leak does not reveal any obvious issue.

The MANIFEST file contains 5 files which are not regenerated by auto_include from Makefile.PL:

inc/Module/Install/Repository.pm
inc/Module/Install/TestBase.pm
inc/Test/Base/Filter.pm
inc/Test/Builder.pm
inc/Test/Builder/Module.pm

Apparently the following patch to Makefile.PL fixes the "problem":

diff --git a/Makefile.PL b/Makefile.PL
index 9f69a22..3764f67 100644
--- a/Makefile.PL
+++ b/Makefile.PL
@@ -9,15 +9,15 @@ requires 'MouseX::Foreign';
 requires 'Object::Event'     => '1.0';
 requires 'Object::Container' => '0.03002';

-test_requires 'Test::More' => '0.92';
 test_requires 'Test::Exception';
 test_requires 'Test::Deep';
-test_requires 'Test::Base';
 test_requires 'Test::TCP' => '0.08';

 tests('t/*.t');
 author_tests('xt');

+use_test_base;
+auto_set_repository;
 auto_include;

However I'm not sure which way to fix the problem is better (the current Makefile.PL fixed by changing the MANIFEST or the patched Makefile.PL with the current MANIFEST)

Some CPAN testers demonstrate a race condition in 04_reconnect.t. Somehow on my development VM it happens every time:

http://www.cpantesters.org/cpan/report/0d4eba30-2867-11e7-8188-e43c188d0c79

Basically connection from AnyEvent::Gearman::Client is rejected earlier than the server part starts.

The fix I use (I'll make a PR later) is to put a wait_port call before we attempt a connection.

Unlike other task queue clients, AnyEvent::Gearman::Client doesn't automatically reconnect