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.

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

[Tizen] Modify dnssd resolve operation process #37003

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
+31 −27
Open

[Tizen] Modify dnssd resolve operation process #37003

Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
051d0f8
[Tizen] Modify dnssd resolve operation process
hyunuktak Jan 9, 2025
12bcab0
Restyled by clang-format
restyled-commits Jan 9, 2025
5a4e247
Modify the return method
hyunuktak Jan 13, 2025
97b87b2
Restyled by clang-format
restyled-commits Jan 13, 2025
75f4269
Fix a segfault
hyunuktak Jan 13, 2025
c6feed9
Readability improvement
hyunuktak Feb 3, 2025
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
58 changes: 31 additions & 27 deletions src/platform/Tizen/DnssdImpl.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -281,12 +281,7 @@ gboolean OnResolveFinalize(gpointer userData)
ChipLogDetail(DeviceLayer, "DNSsd %s", __func__);
auto rCtx = reinterpret_cast<chip::Dnssd::ResolveContext *>(userData);

{
// Lock the stack mutex when calling the callback function, so that the callback
// function could safely perform message exchange (e.g. PASE session pairing).
chip::DeviceLayer::StackLock lock;
rCtx->Finalize(CHIP_NO_ERROR);
}
rCtx->Finalize(CHIP_NO_ERROR);
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we get a description in the summary? You updated the testing section, however the description is still vague: In multi-thread environment, Tizen dnssd's resolve operation process may cause deadlock..

it seems the deadlock is probably because the stack lock already being aquired. Why is that? Why is the comment that was here before (saying this must be locked) not applicable? Should we have a assertChipStackLockedByTheCurrentThread in here instead since we assume already locked? There should be a comment why we know this is already called in the chip stack context.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Perhaps the reason for calling stack lock was for safety in callback function. However, if the app and service (e.g., smartthings) already make a stack lock and call, the possibility of a deadlock due to a double stack lock call was found during the asynchronous process.


rCtx->mInstance->RemoveContext(rCtx);
return G_SOURCE_REMOVE;
Expand Down Expand Up @@ -377,11 +372,38 @@ void OnResolve(dnssd_error_e result, dnssd_service_h service, void * userData)

CHIP_ERROR ResolveAsync(chip::Dnssd::ResolveContext * rCtx)
{
int ret;

ChipLogDetail(DeviceLayer, "DNSsd %s", __func__);

int ret = dnssd_resolve_service(rCtx->mServiceHandle, OnResolve, rCtx);
VerifyOrReturnValue(ret == DNSSD_ERROR_NONE, TizenToChipError(ret),
ChipLogError(DeviceLayer, "dnssd_resolve_service() failed: %s", get_error_message(ret)));
if (rCtx->mInterfaceId == 0)
{
ret = dnssd_create_remote_service(rCtx->mType, rCtx->mName, nullptr, &rCtx->mServiceHandle);
}
else
{
char iface[IF_NAMESIZE + 1] = "";
if (if_indextoname(rCtx->mInterfaceId, iface) == nullptr)
{
ChipLogError(DeviceLayer, "if_indextoname() failed: %s", strerror(errno));
return CHIP_ERROR_POSIX(errno);
}

ret = dnssd_create_remote_service(rCtx->mType, rCtx->mName, iface, &rCtx->mServiceHandle);
}

if (ret != DNSSD_ERROR_NONE)
{
ChipLogError(DeviceLayer, "dnssd_create_remote_service() failed: %s", get_error_message(ret));
return TizenToChipError(ret);
}

ret = dnssd_resolve_service(rCtx->mServiceHandle, OnResolve, rCtx);
if (ret != DNSSD_ERROR_NONE)
{
ChipLogError(DeviceLayer, "dnssd_resolve_service() failed: %s", get_error_message(ret));
return TizenToChipError(ret);
}

rCtx->mIsResolving = true;
return CHIP_NO_ERROR;
Expand Down Expand Up @@ -628,30 +650,12 @@ CHIP_ERROR DnssdTizen::Resolve(const DnssdService & browseResult, chip::Inet::In
std::string fullType = GetFullType(browseResult.mType, browseResult.mProtocol);
auto interfaceId = interface.GetPlatformInterface();
CHIP_ERROR err = CHIP_NO_ERROR;
int ret;

ChipLogDetail(DeviceLayer, "DNSsd %s: name: %s, type: %s, interfaceId: %u", __func__, browseResult.mName, fullType.c_str(),
interfaceId);

auto resolveCtx = CreateResolveContext(browseResult.mName, fullType.c_str(), interfaceId, callback, context);

if (interfaceId == 0)
{
ret = dnssd_create_remote_service(fullType.c_str(), browseResult.mName, nullptr, &resolveCtx->mServiceHandle);
}
else
{
char iface[IF_NAMESIZE + 1] = "";
VerifyOrExit(if_indextoname(interfaceId, iface) != nullptr,
ChipLogError(DeviceLayer, "if_indextoname() failed: %s", strerror(errno));
err = CHIP_ERROR_POSIX(errno));
ret = dnssd_create_remote_service(fullType.c_str(), browseResult.mName, iface, &resolveCtx->mServiceHandle);
}

VerifyOrExit(ret == DNSSD_ERROR_NONE,
ChipLogError(DeviceLayer, "dnssd_create_remote_service() failed: %s", get_error_message(ret));
err = TizenToChipError(ret));

err = DeviceLayer::PlatformMgrImpl().GLibMatterContextInvokeSync(ResolveAsync, resolveCtx);
SuccessOrExit(err);

Expand Down
Loading