search

tpunt / pht

A new threading extension for PHP
BSD 3-Clause "New" or "Revised" License
179 stars 14 forks source link

is this ext production ready #4

Open ClosetGeek-Git opened 5 years ago

tpunt commented 5 years ago

In the projects I have used this extension in, it seems pretty stable. The only thing that is perhaps slightly unstable is the API, since I'm still not 100% sure on what API to expose for the data structures. But I'm mostly waiting for feedback from users on this for now.

So I don't see any issues with using this in production.

joeyhub commented 5 years ago

I can't comment on stability as I've not actually tried running it yet. I very much doubt however that it's entirely stable just because of limited users.

On the other hand looking at the code, the quality of code there really isn't that bad, well organized, sane, informed but also not a vast amount. Basically the question there is, is it super hard to get it to a point where it could be declared to have a very high degree of stability? My opinion is that it's probably not far from that.

There are some todos in there and I also see some questionable bits like possible missing returns in cases of errors as well as some suspiciously repetitive code (could maybe be deduped) so it does look like it might need a little bit of work.

Just looking at calls to things like zend_throw_exception without a return... likely those cases hardly every really happen so don't get noticed. Easy mistake to make I guess programming on instinct. Of course no exception is actually thrown in the C and it's not possible for the function to cause the interpreted to resume as otherwise it would be a callstack overflow.

If you did start using it, it doesn't look to me like it would be very hard to fix bugs as long as easily reproducible or linked to something but you never entirely know for sure.

There's also the question of how active maintenance can be after more than a year though you can fork.

In terms of the API, its functionality might limit some cases. Some of the examples as well I think can bomb out a core (use all it's cycles instead of waiting). Those examples are definitely not necessarily production ready if that turns out to be the case but if they are CPU hogs there are ways around that which entail odd usage of mutex locks really more for syncronization than anything else.

A thought that comes to mind is that if that problem turns out to be real and it's in the examples as well, it's possible the extension which also uses mutexes could suffer the same flaw?

You can't join multiple threads which might pose an issue in scenarios where you need to work with lots and lots of threads especially in complex or unpredictable ways.

If you look at the license, even if it does reach a state where it could be considered production ready, as is the standard the user is still fully responsible for anything that happens.

tpunt commented 5 years ago

Edit Just seen the other issue you opened up - I will look through it now. Same comments still apply to other problems where example code would be useful.

There's also the question of how active maintenance can be after more than a year though you can fork.

This project is (somewhat) being actively maintained. For the most part, the functionality is finished. As you mentioned above with error handling, there may be some edge cases that need cleaning up. The next big task for this project is to make it compatible with PHP 7.3 (which I will be working on soon). The few todos scattered in the code base (like back tracking in hash tables) are definitely not a priority - just something to think about.

In terms of the API, its functionality might limit some cases.

If you can provide code examples of what you would like to achieve, then please do open new issues with such feature requests.

Some of the examples as well I think can bomb out a core (use all it's cycles instead of waiting).

If you can show a code example for this problem, then I can look into rewriting the necessary piece of internal code to use a semaphore (or poll/select).

You can't join multiple threads which might pose an issue in scenarios where you need to work with lots and lots of threads especially in complex or unpredictable ways.

I don't understand this point. Do you have a code example to demonstrate what you would like to achieve?

Ultimately, whilst I haven't found any further bugs since the project was released, getting more users will definitely help to find remaining edge cases to stabilise things further. If you (or anybody else) would like to help with that, then that would be good.

joeyhub commented 5 years ago

I think production ready is very subjective. Ultimately it's really up to the user. This ticket didn't put in the standard. My standard is very high. It might not really be as high for the author of this ticket or others than come along.

My standard is something rock solid in terms of both stability and performance that also delivers the full scope of possibilities for handling scenarios with threading. Rock solid for stability for me means basically if it goes wrong, I did something wrong. There's a particular area of concern.

The terrifying thought of hidden bugs you don't see (that wont produce an explicit error). Data corruption, mysteriously hung processes or ghost threads, etc. I haven't seen any of those but it's a risk.

Looking at the code, it's not too bad in terms of the level of quality I sometimes see (anyone can have a go at C and extension writing, I've seen some things out there that are just ugh, but this it at a reasonable level of quality it can be easily taken further). However it's still not at the level yet.

Lots of things I'm looking at and going I'm not so sure about that. Actually you're quite expressive in the code as well saying you're not sure about stuff in comments. So a lot of unresolved uncertainties here and there. I am also seeing a few potential issues. A few things are just a matter of better DRY, less indirection, KISS, etc. As well as a few performance issues probably on things that'll barely ever run.

The code example with bombing out cores is your code example at the end I suspect :D. If you set the number of iterations to really high and mess with it a bit. I did raise another issue looking at that though that one raises more than one concern, both plexing and syncronisation. Best to keep that to there.

I guess really the thing with a lot of this stuff, it's a case where tests, that fail, might be particularly useful. Issues as a PR with the failing tests might certainly work better at least in some cases. Temporal cases are a bit more tricky, obviously the thread scheduling isn't deterministic so you have to probably do some funky things to excite those potentials and also to detect their effect. Similarly, users wanting some standard of production ready really should consider determine that with their tests in my opinion because of needs are simple it might be alright.

Talking about the issue in examples also being in the code, I think I might have just hit a small case of that which I'll put in the plexing ticket. I'd consider both the issues, no plexing and limitations on syncronisation to full under the same umbrella.