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
Bug fix/default stack size on osx too small #5265
Conversation
…-fix/default-stack-size-on-osx-too-small
…f adjusting the stack size
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please:
- Add Proper ErrorHandling in Production Code
TRI_ASSERT(false) alone is no option. - Fix variable name convention
- Where is thread.join()
lib/Basics/threads-posix.cpp
Outdated
auto err = pthread_attr_init (&stackSizeAttribute); | ||
if (err) { | ||
// We should never be here | ||
TRI_ASSERT(false); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we add a proper exception here please.
This will fail in our tests, but in delivered versions if it shows up (even if it is super unlikely obviously it is still possible) we will end up in undefined behaviour otherwise.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. I also considered this as ultra unlikely and
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
lib/Basics/threads-posix.cpp
Outdated
err = pthread_attr_getstacksize(&stackSizeAttribute, &stackSize); | ||
if (err) { | ||
// We should never be here | ||
TRI_ASSERT(false); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here add Proper error handling
lib/Basics/threads-posix.cpp
Outdated
err = pthread_attr_setstacksize (&stackSizeAttribute, 8388608); | ||
if (err) { | ||
// We should never be here | ||
TRI_ASSERT(false); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
tests/main.cpp
Outdated
} | ||
int result() {return result_;} | ||
private: | ||
Function f_; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i thought our convention for private variables is:
_private
not:
private_
please fix
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
std::thread subthread(runTests, argc, argv); | ||
subthread.join(); | ||
TestThread<decltype(tests)> t(std::move(tests), argc, argv); | ||
result = t.result(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Our of curiousity don't we need the join()
some where?
I do not see any place waiting for the test to complete. On the old we did wait for the subthread.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No don't need join. The thread is joined or detatched in destruction of arangodb::Thread.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Is the error reported by @dothebart sorted out as well? |
ok from internal discussion this has been sorted out, was an issue on the machine. |
OSX default stack size for threads is at 512k, needs tweaking.