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

Jump to bottom

Replace C++ CountDownLatch with std::promise #1854

Merged
merged 3 commits into from
Feb 27, 2024
Merged

Replace C++ CountDownLatch with std::promise #1854

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
732fd08
Replace C++ CountDownLatch with std::promise
pepone Feb 26, 2024
496d38c
Remove Dispatcher & DispatcherCall
pepone Feb 27, 2024
5ee9a40
Revert "Remove Dispatcher & DispatcherCall"
pepone Feb 27, 2024
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
45 changes: 0 additions & 45 deletions cpp/include/IceUtil/CountDownLatch.h
View file
Open in desktop

This file was deleted.

42 changes: 7 additions & 35 deletions cpp/src/Glacier2Lib/SessionHelper.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@
#include <Glacier2/SessionHelper.h>

#include <IceUtil/IceUtil.h>
#include <IceUtil/CountDownLatch.h>
#include <Ice/Ice.h>

#include <algorithm> // required by max
Expand Down Expand Up @@ -683,48 +682,21 @@ SessionHelperI::dispatchCallback(const Ice::DispatcherCallPtr& call, const Ice::
}
}

namespace
{

class DispatcherCallWait : public Ice::DispatcherCall
{

public:

DispatcherCallWait(IceUtilInternal::CountDownLatch& cdl, const Ice::DispatcherCallPtr& call) :
_cdl(cdl),
_call(call)
{
}

virtual void
run()
{
_call->run();
_cdl.countDown();
}

private:

IceUtilInternal::CountDownLatch& _cdl;
const Ice::DispatcherCallPtr _call;
};

}

void
SessionHelperI::dispatchCallbackAndWait(const Ice::DispatcherCallPtr& call, const Ice::ConnectionPtr& conn)
{
if(_initData.dispatcher)
{
IceUtilInternal::CountDownLatch cdl(1);
auto callWait = make_shared<DispatcherCallWait>(cdl, call);
_initData.dispatcher([callWait]()
promise<void> dispatchPromise;

_initData.dispatcher(
[&dispatchPromise, call = std::move(call)]()
{
callWait->run();
call->run();
dispatchPromise.set_value();
Comment on lines +695 to +696
Copy link
Member

Choose a reason for hiding this comment

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

Is this run call guaranteed to never throw?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think so. This keeps the previous behavior. Maybe is more correct to catch the exception and use it to complete the promise.

Copy link
Member

Choose a reason for hiding this comment

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

As far as I can tell, Ice::DispatcherCallPtr is a C++98 API. Why are we still using it?

https://doc.zeroc.com/ice/3.7/client-server-features/the-ice-threading-model/dispatching-requests-to-user-threads#227755398f6b9995a15f4b74982e395cc4afef7b

Copy link
Member Author

Choose a reason for hiding this comment

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

Seems SessionHelper is the only place where we still use it. I will remove it.

Copy link
Member

@bernardnormier bernardnormier Feb 27, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

According to the Ice Manual:

The call given to the dispatcher takes no parameters, returns nothing, and never throws any exception back to the dispatcher.

We should mark it noexcept. So I wouldn't worry about the exception is throws.

Copy link
Member

Choose a reason for hiding this comment

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

And in this particular case, all the "callbacks" are SessionHelper code, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

The callbacks call to the user provided SessionCallback.

},
conn);
cdl.await();
dispatchPromise.get_future().wait();
}
else
{
Expand Down
171 changes: 0 additions & 171 deletions cpp/src/Ice/CountDownLatch.cpp
View file
Open in desktop

This file was deleted.

1 change: 0 additions & 1 deletion cpp/src/Ice/msbuild/ice/ice.vcxproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -515,7 +515,6 @@
<ClCompile Include="..\..\FixedRequestHandler.cpp" />
<ClCompile Include="..\..\Connector.cpp" />
<ClCompile Include="..\..\ConnectRequestHandler.cpp" />
<ClCompile Include="..\..\CountDownLatch.cpp" />
<ClCompile Include="..\..\DefaultsAndOverrides.cpp" />
<ClCompile Include="..\..\DispatchInterceptor.cpp" />
<ClCompile Include="..\..\DLLMain.cpp" />
Expand Down
3 changes: 0 additions & 3 deletions cpp/src/Ice/msbuild/ice/ice.vcxproj.filters
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -285,9 +285,6 @@
<ClCompile Include="..\..\ConnectRequestHandler.cpp">
<Filter>Source Files</Filter>
</ClCompile>
<ClCompile Include="..\..\CountDownLatch.cpp">
<Filter>Source Files</Filter>
</ClCompile>
<ClCompile Include="..\..\DefaultsAndOverrides.cpp">
<Filter>Source Files</Filter>
</ClCompile>
Expand Down
4 changes: 1 addition & 3 deletions cpp/src/IceUtil/msbuild/iceutil/iceutil.vcxproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,7 +102,6 @@
<ItemGroup>
<ClInclude Include="..\..\..\..\include\IceUtil\Config.h" />
<ClInclude Include="..\..\..\..\include\IceUtil\ConsoleUtil.h" />
<ClInclude Include="..\..\..\..\include\IceUtil\CountDownLatch.h" />
<ClInclude Include="..\..\..\..\include\IceUtil\CtrlCHandler.h" />
<ClInclude Include="..\..\..\..\include\IceUtil\DisableWarnings.h" />
<ClInclude Include="..\..\..\..\include\IceUtil\Exception.h" />
Expand All @@ -119,7 +118,6 @@
<ClInclude Include="..\..\..\..\include\IceUtil\ScannerConfig.h" />
<ClInclude Include="..\..\..\..\include\IceUtil\StringConverter.h" />
<ClInclude Include="..\..\..\..\include\IceUtil\StringUtil.h" />
<ClInclude Include="..\..\..\..\include\IceUtil\Thread.h" />
<ClInclude Include="..\..\..\..\include\IceUtil\ThreadException.h" />
<ClInclude Include="..\..\..\..\include\IceUtil\Timer.h" />
<ClInclude Include="..\..\..\..\include\IceUtil\UndefSysMacros.h" />
Expand All @@ -128,4 +126,4 @@
<Import Project="$(VCTargetsPath)\Microsoft.Cpp.targets" />
<ImportGroup Label="ExtensionTargets">
</ImportGroup>
</Project>
</Project>
5 changes: 1 addition & 4 deletions cpp/src/IceUtil/msbuild/iceutil/iceutil.vcxproj.filters
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,9 +56,6 @@
<ClInclude Include="..\..\..\..\include\IceUtil\Config.h">
<Filter>Header Files</Filter>
</ClInclude>
<ClInclude Include="..\..\..\..\include\IceUtil\CountDownLatch.h">
<Filter>Header Files</Filter>
</ClInclude>
<ClInclude Include="..\..\..\..\include\IceUtil\CtrlCHandler.h">
<Filter>Header Files</Filter>
</ClInclude>
Expand Down Expand Up @@ -126,4 +123,4 @@
<Filter>Header Files</Filter>
</ClInclude>
</ItemGroup>
</Project>
</Project>