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.

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

Fix hang queue on open due to panic on atomic op #31

Merged
merged 4 commits into from
Jan 21, 2019

Conversation

urso
Copy link

@urso urso commented Jan 20, 2019

The atomic transaction counter needs to be aligned to 64bit words, so to
not cause a panic on some architectures (arm or 32bit x86)

Unfortunately the file lock was not released when this panic occured,
making applications hang on startup.

We move the atomic to the top of the file and also ensure the file lock
is correctly released (using defer) if an error or panic occurs on
transaction begin.

urso added 2 commits January 20, 2019 03:08
The atomic transaction counter needs to be aligned to 64bit words, so to
not cause a panic on some architectures (arm or 32bit x86)

Unfortunately the file lock was not released when this panic occured,
making applications hang on startup.

We move the atomic to the top of the file and also ensure the file lock
is correctly released (using defer) if an error or panic occurs on
transaction begin.
Copy link
Member

@andrewkroh andrewkroh left a comment

Choose a reason for hiding this comment

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

LGTM

@urso
Copy link
Author

urso commented Jan 21, 2019

I tested with and without the change on ARM 32bit with go1.10 and go1.11. Panic on atomic access did not occur when go-txfile is compiled with go1.11.

@urso urso merged commit aa4be63 into elastic:master Jan 21, 2019
urso pushed a commit that referenced this pull request Jan 24, 2019
We introduce support for testing other environments here. For now we only add ARM (32bit) support to our tests.

The Travis configs are updated to cross-compile and run the testsuite on ARM, via qemu. I've been able to reproduce the panic on atomic access, which has been fixed in #31 with these tests + some false assumption in an unit test has been fixed as well.

As the build and testing environment has become somewhat more complicated due to the need to cross compile and run/test via qemu, we introduce mage here. We hope for the full test suite and cross-combile/test support to be executable via Windows, Linux, and Darwin. We still target x86_64 dev environments, though.
@urso urso deleted the atomic-hangs branch April 17, 2019 03:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants