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

[python] Call_method in service now allows threads. #1760

Merged
merged 1 commit into from
Oct 2, 2024

Conversation

Peguen
Copy link
Contributor

@Peguen Peguen commented Oct 2, 2024

Description

If you used a service client and server in the same process (like described in this issue: #1755), the application locks. With this fix, it does not lock anymore and function as expected.

@Peguen Peguen added cherry-pick-to-support/v5.12 Cherry pick these changes to support/v.5.12 cherry-pick-to-support/v5.13 Cherry pick these changes to support/v5.13 labels Oct 2, 2024
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

clang-tidy made some suggestions

@@ -813,7 +813,9 @@ PyObject* client_call_method(PyObject* /*self*/, PyObject* args) // (client_ha
PyArg_ParseTuple(args, "nsy#i", &client_handle, &method_name, &request, &request_len, &timeout);

bool called_method{ false };
called_method = client_call_method(client_handle, method_name, request, (int)request_len, timeout);
Py_BEGIN_ALLOW_THREADS
called_method = client_call_method(client_handle, method_name, request, (int)request_len, timeout);
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: variable 'called_method' is not initialized [cppcoreguidelines-init-variables]

Suggested change
called_method = client_call_method(client_handle, method_name, request, (int)request_len, timeout);
called_method = 0 = client_call_method(client_handle, method_name, request, (int)request_len, timeout);

Copy link
Contributor

@KerstinKeller KerstinKeller left a comment

Choose a reason for hiding this comment

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

Somehow we missed that 👍

@KerstinKeller KerstinKeller merged commit 974964f into master Oct 2, 2024
24 checks passed
eclipse-ecal-bot pushed a commit that referenced this pull request Oct 2, 2024
)

If service and client are located in the same process, the previous implementation of `call_method` caused deadlocks due to the GIL.
eclipse-ecal-bot pushed a commit that referenced this pull request Oct 2, 2024
)

If service and client are located in the same process, the previous implementation of `call_method` caused deadlocks due to the GIL.
KerstinKeller pushed a commit that referenced this pull request Oct 4, 2024
) (#1762)

If service and client are located in the same process, the previous implementation of `call_method` caused deadlocks due to the GIL.

Co-authored-by: Peguen <[email protected]>
KerstinKeller pushed a commit that referenced this pull request Oct 4, 2024
) (#1761)

If service and client are located in the same process, the previous implementation of `call_method` caused deadlocks due to the GIL.

Co-authored-by: Peguen <[email protected]>
@KerstinKeller KerstinKeller deleted the hotfix/python_service_call_method branch October 9, 2024 12:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cherry-pick-to-support/v5.12 Cherry pick these changes to support/v.5.12 cherry-pick-to-support/v5.13 Cherry pick these changes to support/v5.13
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants