interesting bug, a possibly stupid question: Any idea why isn't this a more commonly seen problem? 

I would have imagined if the code wasn't handling that scenario correctly, then other servers that dynamically allocate threads would have seen similar issues.

I would have expected that it would be seen more often as well.  But one of the people here suggested that perhaps other developers create a thread and reuse it, only creating new threads for larger amounts of work - and not deleting the existing ones.  I don't see a lot of thread questions and bugs posted so I can't speak to how much threads are used.  Except that I was told that 5.x threading is no longer supported and not until 7.2 has threading really been functional.  

Thanks,
Bob
rperper@litespeedtech.com

Thanks for the tests. Actually, that's an interesting experiment, from the usability perspective it seems however to make a little sense. Once a thread is started, it serves multiple requests, not just one. The most meaningful parallel in this case were probably to check how Apache handles it with MinSpareThreads/MaxSpareThreads.

There might be also different handling in regard to whether an MPM uses multiple processes, but i haven't seen so far that threads would be joined until the process exit. When looking it from the MPM implementation perspective - it seems also logic, as a MPM cannot know about implementation details of a concrete SAPI module. Until MPM also doesn't provide callback for thread cleanups, it should not join threads for this reason.

Now, I don't know the exact features the litespeed threading provides, but with the regard to the current TSRM implementation the test code doesn't seem correct. Once a thread() is called, it simply should start a new serving instance. After that it can accept clients and serve multiple requests per process_req(). Furthermore, the lone thread(NULL) calls in the test code look wrong, they should only be called in the new thread context so the correct TLS items are used.

At this point - perhaps the TSRM layer could be extended with the thread cleanup callbacks for the case a server module provides such a functionality, but otherwise the functionality is OK when sage of the TSRM API is sufficient.

Thanks.

We consider this to be a critical path problem as there's no reasonable way in our code to have a thread never terminate.  So we'll be generating a patch to allow cleanup of resources when a thread terminates and posting it to this problem within the next couple of days.  We really hope it can be included in the next PHP patch release.  Please let me know if this is a problem for you.  Thanks much.

Bob Perper
rperper@litespeedtech.com

@rperper please lets see the patch first then. Whether it's suitable for a patch version depends on the extent and severity. As general principles, a patch that targets a stable branch has to retain BC and not breach binary compatibility. Obviously it also should be ensured CLI and Apache still work. Depending on all that, an approval through RM could be required. Otherwise, you could target master.

Thanks.

We were able to use the existing ts_free_thread() call and it seems to have addressed our problem.  I'm closing the problem as we're happy here.

Thanks again,

Bob Perper
rperper@litespeedtech.com

Even better then :)