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

Add stress test for many invocations and fix memory leak #641

Merged
merged 3 commits into from
Dec 9, 2021

Conversation

mbrandenburger
Copy link
Contributor

Add another stress test to detect memory leaks.

@mbrandenburger mbrandenburger added the Test This issue is related to testing label Dec 7, 2021
@mbrandenburger
Copy link
Contributor Author

For some reason my test fails but did not fail the entire CI test ... 🤔

@mbrandenburger
Copy link
Contributor Author

mbrandenburger commented Dec 8, 2021

EDITED: I suspect the changes made in #637 causing the enclave crash (earlier). Running the test without a949ca9 seems to pass.

@mbrandenburger mbrandenburger changed the title Add stress test for many invocations Add stress test for many invocations and fix memory leak Dec 8, 2021
@mbrandenburger mbrandenburger added the bug Something isn't working label Dec 8, 2021
@mbrandenburger
Copy link
Contributor Author

mbrandenburger commented Dec 8, 2021

The stress test runs by default with 1000 invocations (defaultInvocationCount = 1000).
Via export STRESS_TEST_INVOCATION_COUNT=10000 you can set the invocation count when testing locally with larger numbers.

Example:

export STRESS_TEST_INVOCATION_COUNT=10000
cd $FPC_PATH/integration
make stress_test

This should run for a while. Note that the go test framework aborts when exceeding a test run with more than 10min.

@mbrandenburger
Copy link
Contributor Author

mbrandenburger commented Dec 8, 2021

EDITED: I suspect the changes made in #637 causing the enclave crash (earlier). Running the test without a949ca9 seems to pass.

As mentioned earlier, the changes in #637 introduced a memory leak when decoding the creator certificate. Additionally, some pb_release calls were missing. Note that also some pb_release calls where missing before #637, in particular those two:

    pb_release(fpc_CleartextChaincodeRequest_fields, &cleartext_cc_request);
    pb_release(fpc_KeyTransportMessage_fields, &key_transport_message);

@mbrandenburger mbrandenburger marked this pull request as ready for review December 8, 2021 18:32
@mbrandenburger mbrandenburger requested a review from a team as a code owner December 8, 2021 18:32
@mbrandenburger
Copy link
Contributor Author

This PR not contains a fix for the memory leaks that causes the FPC enclave to crash after many invocations. @bvavala please have a look. Once this PR is ready and merged, we can look into PR #640.

@mbrandenburger
Copy link
Contributor Author

Just squashed fixups so this PR is ready for merge.

ecc_enclave/enclave/crypto.cpp Show resolved Hide resolved
ecc_enclave/enclave/crypto.cpp Show resolved Hide resolved
ecc_enclave/enclave/crypto.cpp Show resolved Hide resolved
ecc_enclave/enclave/enclave.cpp Show resolved Hide resolved
Signed-off-by: Marcus Brandenburger <[email protected]>
Signed-off-by: Marcus Brandenburger <[email protected]>
- Adding missing free actions

Signed-off-by: Marcus Brandenburger <[email protected]>
Copy link
Contributor

@bvavala bvavala left a comment

Choose a reason for hiding this comment

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

lg - pending CI

@mbrandenburger mbrandenburger merged commit 5febb77 into hyperledger:main Dec 9, 2021
@mbrandenburger mbrandenburger deleted the stress-invocations branch December 9, 2021 22:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working Test This issue is related to testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants