Skip to content

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

Merged
clue merged 1 commit into
reactphp:masterfrom
NikoGrano:patch-1
Mar 27, 2019
Merged

Check if the function is declared before declaring it.#36
clue merged 1 commit into
reactphp:masterfrom
NikoGrano:patch-1

Conversation

@NikoGrano

Copy link
Copy Markdown
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.

@trowski

trowski commented Mar 20, 2019

Copy link
Copy Markdown

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
Copy Markdown
Contributor Author

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

@trowski

trowski commented Mar 20, 2019

Copy link
Copy Markdown

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

@NikoGrano

Copy link
Copy Markdown
Contributor Author

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

@NikoGrano

Copy link
Copy Markdown
Contributor Author

@trowski Confirmed same issue.

image
image

image
image

image

@NikoGrano

Copy link
Copy Markdown
Contributor Author

@WyriHaximus WyriHaximus left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 March 21, 2019 10:17
@WyriHaximus WyriHaximus added this to the v1.5.1 milestone Mar 21, 2019
@NikoGrano

Copy link
Copy Markdown
Contributor Author

@WyriHaximus Thank you!

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

@trowski

trowski commented Mar 21, 2019

Copy link
Copy Markdown

@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
Copy Markdown
Contributor Author

Maybe because itäs used by pool?
image

@NikoGrano

Copy link
Copy Markdown
Contributor Author

@WyriHaximus Requested changes are made.

@WyriHaximus WyriHaximus left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

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

@NikoGrano

Copy link
Copy Markdown
Contributor Author

Sure!

@NikoGrano

Copy link
Copy Markdown
Contributor Author

Done, ty.

@NikoGrano

Copy link
Copy Markdown
Contributor Author

Status? @clue

@WyriHaximus

Copy link
Copy Markdown
Member

Cheers, will tag this shortly 👍

@NikoGrano

Copy link
Copy Markdown
Contributor Author

THANK YOU EVERYBODY!

@NikoGrano NikoGrano deleted the patch-1 branch March 27, 2019 16:08
@WyriHaximus

Copy link
Copy Markdown
Member

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

Development

Successfully merging this pull request may close these issues.

5 participants