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

refactor!: Lazy load binary file #368

Merged
merged 1 commit into from
Nov 29, 2023

Conversation

tienvx
Copy link
Contributor

@tienvx tienvx commented Nov 29, 2023

  • This is a breaking change, but only inside 10.0.0-alpha.x versions
  • Change c-data from managed to unmanaged

@tienvx tienvx requested review from JP-Ellis and YOU54F November 29, 2023 12:57
@@ -27,7 +27,7 @@ public static function createFrom(string $value, bool $nullTerminated = true): s
{
$length = \strlen($value);
$size = $length + ($nullTerminated ? 1 : 0);
$cData = FFI::new("uint8_t[{$size}]");
$cData = FFI::new("uint8_t[{$size}]", false);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is IMPORTANT change.

This change fix this error:

There was 1 failure:

1) BinaryConsumer\Tests\Service\HttpClientServiceTest::testGetImageContent
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-Binary String: 0xffd8ffe000104a46494600010100000100010000ffdb004300030202020202030202020303030304060404040404080606050609080a0a090809090a0c0f0c0a0b0e0b09090d110d0e0f101011100a0c12131210130f101010ffc0000b08000f000f01011100ffc400160001010100000000000000000000000000020304ffc40020100002020300020301000000000000000001020304051112132100223151ffda0008010100003f004d95c94d5ba4b4b3bc1bbefe0aea2348d8725ca7d46b9e4f93d01c8da1fc19223767c98c74b2d5ab61600fe72d374818edf8643f6279876da5247e8035d3b06be3459af83ad12cd5e18dc844083c328e39573e97454a8dc6c3909b0c57665153ab1dc7c45382586f63d64890b36d53d412687b3d068e6473b20f4c7f847cffd9
+Binary String: 0x01000000160000000000000000000000ef000000000000002f2a2a0a20202020202a2057686574686572207468652055524920686173207468652064656661756c7420706f7274206f66207468652063757272656e7420736368656d652e0a20202020202a0a20202020202a20605073725c487474705c4d6573736167655c557269496e746572666163653a3a676574506f727460206d61792072657475726e206e756c6c206f7220746865207374616e6461726420706f72742e2054686973206d6574686f642063616e20626520757365640a20202020202a20696e646570656e64656e746c79206f662074686520696d706c656d656e746174696f6e2e0a20202020202a2f005153ab1dc7c45382586f63d64890b36d53d412687b3d068e6473b20f4c7f847cffd9

/home/tien/Projects/pact/pact-php/example/binary/consumer/tests/Service/HttpClientServiceTest.php:51
  • This change is also needed for compatibility suite. Sooner or later, we need to made this change.
  • In theory, we need to call FFI::free()
    • But the question is: when, and where?
    • I can't see any memory leak. Probably because of this explanation

@tienvx
Copy link
Contributor Author

tienvx commented Nov 29, 2023

I will bypass the review process for this PR

@tienvx tienvx merged commit 282ddb6 into pact-foundation:ffi Nov 29, 2023
20 of 26 checks passed
@tienvx tienvx deleted the lazy-load-binary-file branch November 29, 2023 13:29
@JP-Ellis
Copy link
Contributor

This all looks good for now.

A couple of considerations with the requirements to free string to avoid memory leaks.

  1. The Pact FFI is loaded only for the duration of a test and upon completion of the test, the library is unloaded thereby freeing memory anyway. Since it is very unlikely that end-users will have long-running clients leading to memory bloat, I think this is generally a minor issue.
  2. Having said the above, it is still a best practice to make sure memory is handled correctly. Could it be worth creating a ticket to track this, and fix this at some point in the future?
  3. In Pact Python, I have created a class called an OwnedString. When an instance of an OwnedString is garbage collected, Python will ensure that the string is appropriately freed. Could something similar be done in PHP?

@tienvx
Copy link
Contributor Author

tienvx commented Nov 30, 2023

Thank for taking a look at it. Here is the ticket to track that issue #371

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

Successfully merging this pull request may close these issues.

2 participants