search

chobie / php-uv

libuv php extension
184 stars 21 forks source link

Add uv_stop, signal handling, etc. #53

Closed rdlowrey closed 10 years ago

rdlowrey commented 10 years ago

Please let me know if you have questions or comments -- the commit messages should be self-explanatory.

bwoebi commented 10 years ago

Actually pcntl also defines signal constants, but in global name space. Wouldn't it be possible to just use them or define them if they don't exist? Might confuse people when there exist the two… (As using the pcntl constants then doesn't throw a notice here... It's likely to cause errors)

See also: http://lxr.php.net/xref/phpng/ext/pcntl/pcntl.c#213

Shouldn't be a major problem as pcntl constants have the same value.

chobie commented 10 years ago

@rdlowrey Thanks, I'll look into this tonight. Is this ready to merge?

@bwoebi I prefer php-uv has signal constants as it's not depends pcntl (php-uv doesn't use pcntl functions). You know PHP constants is just alias of the value, Doesn't have type information like other languages. so I guess it's not problem in that case.

rdlowrey commented 10 years ago

@chobie Yes, this should be ready to merge. The failed tests in Travis are unrelated/pre-existing things. I'll likely submit PR's to fix those as well in the coming days.

I added a .phpt test for the uv_stop() function, but was unsure of the best way to test the signal handling code in userland (I did some manual testing FWIW to ensure things work as expected).

I'll be using the extension heavily over the next few weeks so I'll be around to help out and should have a few more PRs coming your way for small features here or there.

Also, I will update the README.md file to reflect the additions in this PR. You may want to hold off on merging this until I do that so you can do it all in a single merge.

bwoebi commented 10 years ago

@chobie Well, I meant it should define the same constants, if they aren't defined yet. I don't see the point in duplicating that information.

rdlowrey commented 10 years ago

The bigger annoyance (and I know @bwoebi has mentioned this previously) is that these constants need to be defined at the extension layer. Multiple extensions require/utilize signal constants and even if the PHP core doesn't expose signal handling it would be useful to have them defined there so there aren't lots of duplicate constants running around :/

chobie commented 10 years ago

@bwoebi Sorry for my poor English. Sometimes I misunderstood sentences. (I've tried to improve my skill but it need some time.)

Yea, It would be nice. But we have to care extension loading order if we support that. For instance: If we load php-uv and pcntl extensions in that order. pcntl will outputs notice like ' PHP Notice: Constant SIGUSR2 already defined in Unknown on line 0'.

Other constants also have this problem (O_RDONLY...). so I think to choose class constants is better way to avoid some problems.

@rdlowrey Thanks explain it :)

bwoebi commented 10 years ago

@chobie as far as I know, statically (pcntl) built-in extensions always load first...

chobie commented 10 years ago

Hmm, In those days, I used dynamic loading pcntl. I've checked almost distribution statically installed pcntl. so it's not real problem.