Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Check if the function is declared before declaring it. #36

Merged
merged 1 commit into from Mar 27, 2019
Merged

Check if the function is declared before declaring it. #36

merged 1 commit into from Mar 27, 2019

Conversation

@NikoGrano
Copy link
Contributor

@NikoGrano NikoGrano commented Mar 20, 2019

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.

@trowski
Copy link

@trowski trowski commented Mar 20, 2019

Sounds like you're inheriting class and function definitions with pthreads, then including the Composer autoloader again. If this is the case, that's the real problem to be fixed.

If there is some other scenario that can lead to this problem, please describe it.

@NikoGrano
Copy link
Contributor Author

@NikoGrano NikoGrano commented Mar 20, 2019

Well, yes I need to include autoloader again. Without it I get undefined error. 🙃
Will be checking few probably causes tomorrow.

@trowski
Copy link

@trowski trowski commented Mar 20, 2019

I recommend using PTHREADS_INHERIT_INI only and then including the Composer autoloader in the thread.

@NikoGrano
Copy link
Contributor Author

@NikoGrano NikoGrano commented Mar 20, 2019

I'm using none currently, but lez see...

@NikoGrano
Copy link
Contributor Author

@NikoGrano NikoGrano commented Mar 21, 2019

@trowski Confirmed same issue.

image
image

image
image

image

@NikoGrano
Copy link
Contributor Author

@NikoGrano NikoGrano commented Mar 21, 2019

Copy link
Member

@WyriHaximus WyriHaximus left a comment

This makes total sense to me but I rather see that we use a functions_include.php like we do in other packages, for example: https://github.com/reactphp/promise-stream/blob/master/src/functions_include.php

@WyriHaximus WyriHaximus requested review from clue and jsor Mar 21, 2019
@WyriHaximus WyriHaximus added this to the v1.5.1 milestone Mar 21, 2019
@NikoGrano
Copy link
Contributor Author

@NikoGrano NikoGrano commented Mar 21, 2019

@WyriHaximus Thank you!

I will make requested changes after I finish some critical work.

@trowski
Copy link

@trowski trowski commented Mar 21, 2019

@Niko9911 There's something else going wrong. We use pthreads in amphp/parallel and have no issues with functions being redeclared, and we have no conditionals around function declarations.

Here is our definition of Thread::run() and where we call Thread::start().

@NikoGrano
Copy link
Contributor Author

@NikoGrano NikoGrano commented Mar 21, 2019

Maybe because itäs used by pool?
image

@NikoGrano
Copy link
Contributor Author

@NikoGrano NikoGrano commented Mar 21, 2019

@WyriHaximus Requested changes are made.

Copy link
Member

@WyriHaximus WyriHaximus left a comment

LGTM 👍

One more thing, could you squash them into one commit?

@NikoGrano
Copy link
Contributor Author

@NikoGrano NikoGrano commented Mar 21, 2019

Sure!

@NikoGrano
Copy link
Contributor Author

@NikoGrano NikoGrano commented Mar 21, 2019

Done, ty.

jsor
jsor approved these changes Mar 21, 2019
@NikoGrano
Copy link
Contributor Author

@NikoGrano NikoGrano commented Mar 26, 2019

Status? @clue

clue
clue approved these changes Mar 27, 2019
@clue clue merged commit d19fe5d into reactphp:master Mar 27, 2019
1 check passed
@WyriHaximus
Copy link
Member

@WyriHaximus WyriHaximus commented Mar 27, 2019

Cheers, will tag this shortly 👍

@NikoGrano
Copy link
Contributor Author

@NikoGrano NikoGrano commented Mar 27, 2019

THANK YOU EVERYBODY!

@NikoGrano NikoGrano deleted the patch-1 branch Mar 27, 2019
@WyriHaximus
Copy link
Member

@WyriHaximus WyriHaximus commented Mar 27, 2019

Ok v1.5.1 has been released

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants