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

Remove unused beast::currentTimeMillis() #2345

Closed
wants to merge 1 commit into from
Closed

Remove unused beast::currentTimeMillis() #2345

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
5 changes: 0 additions & 5 deletions Builds/VisualStudio2015/RippleD.vcxproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1651,11 +1651,6 @@
</ClCompile>
<ClInclude Include="..\..\src\ripple\beast\core\SystemStats.h">
</ClInclude>
<ClCompile Include="..\..\src\ripple\beast\core\Time.cpp">
<ExcludedFromBuild>True</ExcludedFromBuild>
</ClCompile>
<ClInclude Include="..\..\src\ripple\beast\core\Time.h">
</ClInclude>
<ClCompile Include="..\..\src\ripple\beast\core\WaitableEvent.cpp">
<ExcludedFromBuild>True</ExcludedFromBuild>
</ClCompile>
Expand Down
6 changes: 0 additions & 6 deletions Builds/VisualStudio2015/RippleD.vcxproj.filters
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2283,12 +2283,6 @@
<ClInclude Include="..\..\src\ripple\beast\core\SystemStats.h">
<Filter>ripple\beast\core</Filter>
</ClInclude>
<ClCompile Include="..\..\src\ripple\beast\core\Time.cpp">
<Filter>ripple\beast\core</Filter>
</ClCompile>
<ClInclude Include="..\..\src\ripple\beast\core\Time.h">
<Filter>ripple\beast\core</Filter>
</ClInclude>
<ClCompile Include="..\..\src\ripple\beast\core\WaitableEvent.cpp">
<Filter>ripple\beast\core</Filter>
</ClCompile>
Expand Down
24 changes: 19 additions & 5 deletions src/ripple/app/main/Main.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,6 @@
#include <ripple/protocol/BuildInfo.h>
#include <ripple/beast/clock/basic_seconds_clock.h>
#include <ripple/beast/core/CurrentThreadName.h>
#include <ripple/beast/core/Time.h>
#include <ripple/beast/utility/Debug.h>

#include <beast/unit_test/dstream.hpp>
Expand All @@ -57,6 +56,10 @@
#include <utility>
#include <stdexcept>

#ifdef _MSC_VER
#include <sys/types.h>
#include <sys/timeb.h>
#endif

#if BOOST_VERSION >= 106400
#define HAS_BOOST_PROCESS 1
Expand Down Expand Up @@ -588,11 +591,22 @@ int run (int argc, char** argv)
//
int main (int argc, char** argv)
{
// Workaround for Boost.Context / Boost.Coroutine
// https://svn.boost.org/trac/boost/ticket/10657
(void)beast::currentTimeMillis();
Copy link
Collaborator

Choose a reason for hiding this comment

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

From the comment, the purpose of this code is to call GetTimeZoneInformation before the first coroutine is launched. The bug is marked as "wontfix". So don't we still need this call?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That's a great question. It's entirely unclear to me how this call: https://github.com/ripple/rippled/pull/2345/files#diff-0da1f01eb51089298d4839f18d92fa41L41 could have the side effect of loading time zone information. But this would not be the only example of twisted software. How would we determine whether we still need the call? I'd prefer not to leave it in place simply because we're afraid to remove it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Seems like a windows only issue...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Do we know what the failure mode would be? Perhaps @bachase can try building this branch and see if it behaves well for him, since he runs Windows.

Copy link
Collaborator

Choose a reason for hiding this comment

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

It looks like it happens on the windows _f_time call. The timeptr struct contains timezone information: https://msdn.microsoft.com/en-us/library/z54t9z5f.aspx

I'd prefer to leave this in, even if coro's don't currently call GetTimezoneInformation. If it's added later it would be a PITA to track down the crash.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I prefer not to leave it in it's current form. If it's a Windows only problem we can make a Windows-only call in the #ifdef _MSC_VER immediately below. With a paragraph comment explaining the justification. But I may need to punt and ask someone else on the team to do that, since I don't run a Windows box.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not observing a crash on windows with this change when running the unit tests. Using the example code on the boost ticket, I also do not observe a crash with VS 2017 on Windows 10. I do not have other versions of windows available to test. Even though I'm unable to recreate, I'm with @seelabs and would prefer not have this bite us in the future.

I can try more testing to see if I can recreate, but frankly this seems fairly low priority relative to other work. I'm fine with @scottschurr's suggestion of just moving the code inside the existing windows specific startup block.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks @bachase. I have to agree with your assessment of the priority of this work. I'll make a stab at a Windows only version of the code.


#ifdef _MSC_VER
{
// Work around for https://svn.boost.org/trac/boost/ticket/10657
// Reported against boost version 1.56.0. If an application's
// first call to GetTimeZoneInformation is from a coroutine, an
// unhandled exception is generated. A workaround is to call
// GetTimeZoneInformation at least once before launching any
// coroutines. At the time of this writing the _ftime call is
// used to initialize the timezone information.
struct _timeb t;
#ifdef _INC_TIME_INL
_ftime_s (&t);
#else
_ftime (&t);
#endif
}
ripple::sha512_deprecatedMSVCWorkaround();
#endif

Expand Down
46 changes: 0 additions & 46 deletions src/ripple/beast/core/Time.cpp
View file
Open in desktop

This file was deleted.

35 changes: 0 additions & 35 deletions src/ripple/beast/core/Time.h
View file
Open in desktop

This file was deleted.

1 change: 0 additions & 1 deletion src/ripple/beast/core/core.unity.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -193,7 +193,6 @@
#include <ripple/beast/core/CurrentThreadName.cpp>
#include <ripple/beast/core/SemanticVersion.cpp>
#include <ripple/beast/core/SystemStats.cpp>
#include <ripple/beast/core/Time.cpp>
#include <ripple/beast/core/WaitableEvent.cpp>

#ifdef _CRTDBG_MAP_ALLOC
Expand Down