Skip to content

Conversation

@NikoGrano
Copy link
Contributor

In case you use pthreads or something else related to threading you might encounter this issue. It happens only with functions and is fixable only by adding these checks. Another way to avoid this is to wrap functions as static into classes.

@NikoGrano
Copy link
Contributor Author

@clue Can you take a look ASAP? Big thanks!

@clue
Copy link
Owner

clue commented Mar 20, 2019

@Niko9911 That's interesting, thank you.

Can you gist the problem you're seeing so we can reproduce this?

We've addressed similar problems in @reactphp a longer time ago via reactphp/promise#25 and it looks like Composer fixed this for the most common use cases via composer/composer#4186 in the meantime. If we can still reproduce this problem, I would vote for a more local work around via an additional include file like in the linked PR.

What do you think? 👍

@NikoGrano
Copy link
Contributor Author

I can take a look into this problem again, but pretty sure this cannot be fixed in level of code. About the reproducing, I can take a look. Kinda deeply in the code already. Also you will need ZTS compiled PHP with pthreads.

Ofc, could show remotely the problem, but cannot share the code as that's not my decision to share it. 😞

The problem occurs only in the new child-thread as some of the classes are loaded there and some are not. That means I need to call autoloader once more. Also if I need instance of autoloader for use via require later on this will happen also.

In any case. The best solution would be just to add these checks. If possible? I will be updating this PR to be exactly same as that other.

I'm gonna work tomorrow on gist to reproduce this.

@NikoGrano
Copy link
Contributor Author

Also asking, what you mean by local? Add these checks into the project? You suggest in that to fork this and all other modules required and it doesn't sound good. Thanks but, no thanks.

@trowski
Copy link

trowski commented Mar 20, 2019

Related issue: reactphp/promise-timer#36

@NikoGrano
Copy link
Contributor Author

Confirmed still the issue.

@NikoGrano
Copy link
Contributor Author

@NikoGrano
Copy link
Contributor Author

I think you're aware of reactphp/promise-timer#36

I updated also the PR to work with requested login. What you think? Could it be possible to merge this?

@NikoGrano
Copy link
Contributor Author

Status?

@clue
Copy link
Owner

clue commented Mar 27, 2019

@Niko9911 Changes LGTM, can you squash this into a single commit like in reactphp/promise-timer#36? :shipit:

@clue clue added the bug label Mar 27, 2019
@clue clue added this to the v1.5.0 milestone Mar 27, 2019
@NikoGrano
Copy link
Contributor Author

This is also ready to merge. Thank you. 🏅

@clue
Copy link
Owner

clue commented Mar 27, 2019

Thanks for the quick update, now let's get this in! :shipit:

@clue clue merged commit 46c1830 into clue:master Mar 27, 2019
@NikoGrano NikoGrano deleted the patch-1 branch March 27, 2019 16:55
@NikoGrano
Copy link
Contributor Author

Don't forget to tag 😉

@NikoGrano
Copy link
Contributor Author

@clue Could you tag this as 1.4.1?

@clue clue modified the milestones: v1.5.0, v1.4.1 Apr 9, 2019
@clue clue removed the new feature label Apr 9, 2019
eileenmcnaughton added a commit to eileenmcnaughton/nz.co.fuzion.omnipaymultiprocessor that referenced this pull request Aug 7, 2019
It seems likely that clue/stream-filter#23 is the fix for #13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants