Skip to content

Free the arbitrated client token from client manager #444

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.

Sign up for GitHub

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

Merged
MichalPrincNXP merged 1 commit into from
May 16, 2025
Merged

Free the arbitrated client token from client manager #444

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions erpc_c/infra/erpc_arbitrated_client_manager.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,12 @@ void ArbitratedClientManager::performClientRequest(RequestContext &request)
request.getCodec()->updateStatus(err);
}

if (token != 0)
{
// Also if status bad, need to free the token.
m_arbitrator->removePendingClient(token);
}

#if ERPC_MESSAGE_LOGGING
if (request.getCodec()->isStatusOk() == true)
{
Expand Down
9 changes: 6 additions & 3 deletions erpc_c/infra/erpc_transport_arbitrator.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -193,8 +193,6 @@ erpc_status_t TransportArbitrator::clientReceive(client_token_t token)
// Wait on the semaphore until we're signaled.
info->m_sem.get(Semaphore::kWaitForever);

removePendingClient(info);

return kErpcStatus_Success;
}

Expand Down Expand Up @@ -227,11 +225,16 @@ TransportArbitrator::PendingClientInfo *TransportArbitrator::addPendingClient(vo
return info;
}

void TransportArbitrator::removePendingClient(PendingClientInfo *info)
void TransportArbitrator::removePendingClient(client_token_t token)
{
// Convert token to pointer to info struct.
PendingClientInfo *info = reinterpret_cast<PendingClientInfo *>(token);
Mutex::Guard lock(m_clientListMutex);
PendingClientInfo *node;

erpc_assert((token != 0) && ("invalid client token" != NULL));
erpc_assert((info->m_sem.getCount() == 0) && ("Semaphore should be clean" != NULL));
Copy link

@elupus elupus Oct 13, 2025

Choose a reason for hiding this comment

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

This assert will trigger if we follow the non status ok path here:https://github.com/EmbeddedRPC/erpc/pull/444/files#diff-74a78bba1605a2e491d18277550d2c7db7943691c691d14d7a732558f9a0c681L83. Since then nothing has called received. But something may have set a result.

Copy link
Contributor Author

@amgross amgross Oct 15, 2025

Choose a reason for hiding this comment

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

Sorry @elupus , I don't understand you, can you please explain the flow deeply please?

Copy link

@elupus elupus Oct 15, 2025

Choose a reason for hiding this comment

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

If the arbitrator->send call fails for whatever reason. The buffer will be signalled as failed. However the buffer has already been handed of to handed of to another thread (the receiving thread). This other thread can then just have received data that complete this request or it can just have received some aborting read condition.

The race condition ls very tight with master of erpc (but very much there), if you fix some other race conditions like making sure errors are propagated from the arbitrator, the race condition window grows.

In general, if we handed the buffer to another thread we shall always remove it as well. How it was finalized is nothing that make sense to assert on (warning could be fine)

Copy link
Contributor Author

@amgross amgross Oct 19, 2025

Choose a reason for hiding this comment

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

Sorry @elupus , I still don't understand.

On which of the two asserts you talking, the token or semaphore? and if send failed, why other thread has the buffer (and what is your system, the sender and reciever are on same CPU?)

Is this issue that happened to you or theoretical issue?

Can you please explain step by step, with permalink to the relevant lines?

Copy link

@xianyo xianyo Dec 20, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

This assert will trigger if we follow the non status ok path here:https://github.com/EmbeddedRPC/erpc/pull/444/files#diff-74a78bba1605a2e491d18277550d2c7db7943691c691d14d7a732558f9a0c681L83. Since then nothing has called received. But something may have set a result.

I have the same problem assert,How should we solve it?@elupus

Copy link

@elupus elupus Dec 25, 2025

Choose a reason for hiding this comment

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

I will check how we handle this internally. Since #469 stalled which is semi related i got sidetracked.


// Clear fields.
info->m_request = NULL;
info->m_isValid = false;
Expand Down
14 changes: 7 additions & 7 deletions erpc_c/infra/erpc_transport_arbitrator.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -157,6 +157,13 @@ class TransportArbitrator : public Transport
*/
erpc_status_t clientReceive(client_token_t token);

/*!
* @brief This function free client token.
*
* @param[in] token the client token to remove.
*/
void removePendingClient(client_token_t token);

/*!
* @brief Request info for a client trying to receive a response.
*/
Expand Down Expand Up @@ -200,13 +207,6 @@ class TransportArbitrator : public Transport
*/
PendingClientInfo *addPendingClient(void);

/*!
* @brief This function removes pending client.
*
* @param[in] info Pending client info to remove.
*/
void removePendingClient(PendingClientInfo *info);

/*!
* @brief This function removes pending client list.
*
Expand Down