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

JVM crash #395

Closed
sscarduzio opened this issue Jul 9, 2017 · 7 comments · Fixed by #409
Closed

JVM crash #395

sscarduzio opened this issue Jul 9, 2017 · 7 comments · Fixed by #409
Assignees
Labels
Milestone

Comments

@sscarduzio
Copy link
Contributor

I'm out of ideas about what can ever cause this catastrophic crash, I was hoping some of you can give some pointers on where to look nest.

https://travis-ci.org/sscarduzio/elasticsearch-readonlyrest-plugin/jobs/251713488

Relevant code:

https://github.com/sscarduzio/elasticsearch-readonlyrest-plugin/blob/support_2x/tests-utils/src/main/java/org/elasticsearch/plugin/readonlyrest/utils/containers/ESWithReadonlyRestContainer.java

@rnorth
Copy link
Member

rnorth commented Jul 9, 2017

Ouch. That doesn't look good. Based on the error message, it seems as though there's a failure occurring within the Jansi library's JNI code. fusesource/jansi#66 looks vaguely similar and suggests that there is a clash with the version of Jansi shipped with Gradle.

Do you think this sounds plausible?

It might be worth trying to force Testcontainers to use a different version of Jansi for it's transitive dependency; whichever version Gradle is bringing in (or any other version that may be on your classpath).

I'm sorry that we've never observed this before, otherwise I'd hope we could be of more help! If we can find a way to fix within Testcontainers we'll definitely do that.

@sscarduzio
Copy link
Contributor Author

Yeah I agree, will try to exclude jansi from the testcontainer dependency. But why did this start to happen now? Did you add new dependencies?

@bsideup
Copy link
Member

bsideup commented Jul 9, 2017

@sscarduzio yes, in 1.3.1 we added "pre-flight checks", they use @rnorth's VisibleAssertions, and they have a dependency on jansi. We might try to shade it if it causes issues to the end-user.

P.S. didn't know that you use Testcontainers in readonly-rest-plugin, that's cool 👍

@sscarduzio
Copy link
Contributor Author

sscarduzio commented Jul 9, 2017

Yes please, would be nice if you manage to shade that stuff. Our project build files are already way too tricky to add more exclude clauses.

Yeah we use testcontainers because what we want to prove in CI is that the plugin behaves the same way across all Elasticsearch versions. So we test the behaviour over the (docker) network.
And docker is supported by Travis too, so it's pretty ideal. When it works, of course 😂

Did you know readonlyrest plugin? Wow, I'm flattered. 👍

@bsideup
Copy link
Member

bsideup commented Jul 9, 2017

@sscarduzio sorry to hear that it doesn't work for you sometime :)

I'll take a look at jansi dependency and how can we shade it.

If you need a workaround until that fix, you can just disable the checks, so that they will not use jansi at all. You can add testcontainers.properties file to your classpath with checks.disable=true line.

And thanks for reporting!

@bsideup bsideup added this to the 1.5.0 milestone Jul 9, 2017
@bsideup bsideup self-assigned this Jul 9, 2017
@sscarduzio
Copy link
Contributor Author

Brilliant, will try the workaround right now :)

@sscarduzio
Copy link
Contributor Author

I didn't mean to say TC is bad and breaks all the times, just that it's naturally more fragile because docker operates below the JVM abstraction level and stuff tends to get complicated (i.e. docker version upgrades, different platforms, etc).

I guess it's the price to pay for doing things right. ¯_(ツ)_/¯

sscarduzio added a commit to sscarduzio/elasticsearch-readonlyrest-plugin that referenced this issue Jul 9, 2017
rnorth added a commit that referenced this issue Jul 16, 2017
Removes usage of Jansi - fixes #395
rnorth added a commit that referenced this issue Jul 24, 2017
Removes usage of Jansi - fixes #395
rnorth added a commit that referenced this issue Jul 24, 2017
* Upgrade to v2.0.0 of Visible Assertions
Removes usage of Jansi - fixes #395

* Update changelog
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants