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 #1025 - Stop using Thread._stop incorrectly when stopping ThreadedArbiter. #1052

Closed

Conversation

templed
Copy link
Contributor

@templed templed commented Feb 8, 2018

Also restrict tornado to v<5 (this is done because in v5 the initializer arguments have changed for some of the classes).

I tried using http://issue2pr.herokuapp.com/ to facilitate this PR but it didn't work for me.

It's arguable to either use the dunder syntax or just rearrange the inheritance order of ThreadedArbiter. I went with the former because I prefer it, so I can change what I've done if it's preferable to do the latter :)

Copy link
Contributor

@k4nar k4nar left a comment

Choose a reason for hiding this comment

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

Your PR looks good, but our CI seems to still be broken :( . I have to find some time to fix it.

@coveralls
Copy link

coveralls commented Feb 8, 2018

Coverage Status

Coverage decreased (-0.03%) to 62.94% when pulling a6ae42d on commodityvectors:ThreadedArbiter-stop into 4863345 on circus-tent:master.

@templed templed force-pushed the ThreadedArbiter-stop branch 2 times, most recently from 53e4851 to 47925ed Compare February 8, 2018 19:54
@templed
Copy link
Contributor Author

templed commented Feb 8, 2018

So I spent a bit of time getting the tests to pass. There is still a weird InvocationError with nosetests that I don't understand for the Python 3.5 suite, but my experience is with pytest so I'm somewhat in the dark.

@templed
Copy link
Contributor Author

templed commented Feb 8, 2018

OK I did some more tweaks to the CI setup and on my repo the py34/py35 tests worked (https://travis-ci.org/commodityvectors/circus/builds/339206732) but have failed here for a reason I thought was resolved by the pyenv trick.
I'll leave it for tonight but I'm quite eager to get this merged as that would avoid me having to deal with a local copy of circus.

@dvf
Copy link

dvf commented Mar 12, 2018

It looks like Circus is broken for anyone using pipenv by this, as transitive dependencies are upgraded doing a pipenv lock 😢

@k4nar
Copy link
Contributor

k4nar commented Jun 15, 2018

The main commit was cherry-picked in 29b0bf3. Thank you for your work and sorry for the awful delay!

@k4nar k4nar closed this Jun 15, 2018
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.

4 participants