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

gevent 1.1b4 breaks gevent.signal #648

Closed
sylvinus opened this issue Sep 5, 2015 · 6 comments
Closed

gevent 1.1b4 breaks gevent.signal #648

sylvinus opened this issue Sep 5, 2015 · 6 comments
Labels
Type: Bug Identified as a bug; needs a code change to fix

Comments

@sylvinus
Copy link

sylvinus commented Sep 5, 2015

My app is breaking after an update from 1.1b3 to 1.1b4 because of an API change for gevent.signal, previously a class, now a module.

See usage here:
https://github.com/pricingassistant/mrq/blob/master/mrq/worker.py#L618

I probably won't be the only one depending on this behaviour, so I'd recommend making sure backward compatibility is kept!

Thanks!

@jamadden
Copy link
Member

jamadden commented Sep 5, 2015

gevent.signal was an alias for gevent.hub.signal. Perhaps you can use that directly? I should have called that alias change out more clearly in the release notes, but I didn't even realize it was there; the alias is covered by no test cases.

Due to the way monkey-patching works, stdlib module X needs to be mirrored by a gevent.X module directly; at least, that's certainly the cleanest way, and the way most people expect gevent to work. I'm not sure it's possible to resolve the naming conflict in a nice way, but I'm open to ideas.

@jamadden
Copy link
Member

jamadden commented Sep 5, 2015

It seems like some ugliness with magic proxy objects inserted directly into sys.modules might be able to preserve both behaviours, but it's not clear to me that the ugliness and magic are worth it (since the fix is an easy search and replace, and the breakage is obvious and not subtle). Alternatively, there is the possibility of using some additional magic in monkey to rename the module using a __target__ attribute, but that also introduces long-term magic and naming confusion that's not clearly great either. I'd appreciate more input from anyone affected by this.

@sylvinus
Copy link
Author

sylvinus commented Sep 5, 2015

Thanks! I can move our package to use gevent.hub.signal, no big deal.

I just wanted to make you aware of the potential compatibility break, probably not wanted for a 1.1 release. If nobody else was relying on this behaviour, why bother indeed! However that does not seem to be the case:
https://github.com/search?utf8=%E2%9C%93&q=%22gevent.signal%22&type=Code&ref=searchresults

Maybe a sensible solution would be to add a short-term hack to make gevent.signal callable as you described, with a deprecation warning for a future 2.0? I'll let you be the judge on that!

@jamadden
Copy link
Member

jamadden commented Sep 5, 2015

Thanks for the heads up! It would have been embarrassing indeed to release like that! I'm exploring the possibilities and hope to have something committed within a few days.

@jamadden jamadden added the Type: Bug Identified as a bug; needs a code change to fix label Sep 5, 2015
jamadden added a commit that referenced this issue Sep 15, 2015
@jamadden
Copy link
Member

Thanks for the report. I have updated master with a test case for this scenario, and a proxy object that solves it. If you're able to test a checkout with your application I would appreciate any feedback.

@sylvinus
Copy link
Author

Thanks @jamadden! Can confirm it fixes the issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Bug Identified as a bug; needs a code change to fix
Projects
None yet
Development

No branches or pull requests

2 participants