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

Incompatibility with pthreads #12

Closed
LuckyFellow opened this issue Feb 7, 2019 · 7 comments
Closed

Incompatibility with pthreads #12

LuckyFellow opened this issue Feb 7, 2019 · 7 comments
Assignees

Comments

@LuckyFellow
Copy link

LuckyFellow commented Feb 7, 2019

Environment

Summary

Without threads, operations work as expected.
As soon as operations are performed in a seperate thread, some result in "INF".

Reproducing Code

<?php

$thread = new class() extends Thread {

    public function run() {
        var_dump((new Decimal\Decimal("1"))->mul("9"));
        var_dump((new Decimal\Decimal("1"))->mul("10"));
    }
};

$thread->start() && $thread->join();
?>

Expected Output

object(Decimal\Decimal)#39 (2) {
  ["value"]=>
  string(1) "9"
  ["precision"]=>
  int(28)
}
object(Decimal\Decimal)#42 (2) {
  ["value"]=>
  string(2) "10"
  ["precision"]=>
  int(28)
}

Actual Output

object(Decimal\Decimal)#3 (2) {
  ["value"]=>
  string(1) "9"
  ["precision"]=>
  int(28)
}
PHP Warning:  Loss of data on string conversion in /app/test.php on line 7
object(Decimal\Decimal)#2 (2) {
  ["value"]=>
  string(3) "INF"
  ["precision"]=>
  int(28)
}

@rtheunissen
Copy link
Contributor

Fixed in 48920be, will confirm that it is safe and correct, then release a patch.

@rtheunissen
Copy link
Contributor

Released as v1.1.2 ✨

@LuckyFellow
Copy link
Author

Awesome, thank you very much!
The operations seem to work correctly in different threads now.

Running the "Reproducing Code" from above will issue the following warning now though:

context.c:55: warning: mpd_setminalloc: ignoring request to set MPD_MINALLOC a second time

@rtheunissen
Copy link
Contributor

I'll get on that asap, thanks for the report.

@rtheunissen rtheunissen reopened this Feb 8, 2019
@krakjoe krakjoe self-assigned this Feb 8, 2019
krakjoe added a commit that referenced this issue Feb 8, 2019
  libmpdec assumes that contexts are truly global symbols, they **must**
  be stored as such
  this commit moves the globals outside of the control of zend, and initializes
  them once per process, as the library requires
krakjoe added a commit that referenced this issue Feb 8, 2019
  libmpdec assumes that contexts are truly global symbols, they **must**
  be stored as such
  this commit moves the globals outside of the control of zend, and initializes
  them once per process, as the library requires
@krakjoe
Copy link
Collaborator

krakjoe commented Feb 8, 2019

Hi @rtheunissen ... I read the libmpdec source, it actually does require that contexts are truly global, I committed to both your active branches the fixes for that, hope I didn't make a mess of logs or do it badly ...

@LuckyFellow leaving this for you to close ...

rtheunissen added a commit that referenced this issue Feb 9, 2019
This reverts commit 0bc1cb0.
rtheunissen added a commit that referenced this issue Feb 9, 2019
This reverts commit 86c9f44.
@rtheunissen
Copy link
Contributor

rtheunissen commented Feb 9, 2019

@krakjoe @LuckyFellow I've fixed this a little bit differently in b211485 which has been released as part of 1.2.0. Thanks for your help with this one Joe. 🙏

I'll wait for confirmation before we close this.

@LuckyFellow
Copy link
Author

I can confirm, that there is no warning issued anymore and the operations function properly.

Thank you very much!

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

No branches or pull requests

3 participants