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

Deprecation Warning of Twisted #3288

Open
Aqua-Dream opened this issue Jun 11, 2018 · 10 comments
Open

Deprecation Warning of Twisted #3288

Aqua-Dream opened this issue Jun 11, 2018 · 10 comments
Labels

Comments

@Aqua-Dream
Copy link

Aqua-Dream commented Jun 11, 2018

May you do a little modification to avoid these two warnings?

[py.warnings] WARNING: /.../scrapy/core/downloader/webclient.py:4: DeprecationWarning: twisted.web.client.HTTPClientFactory was deprecated in Twisted 16.7.0: please use https://pypi.org/project/treq/ or twisted.web.client.Agent instead
from twisted.web.client import HTTPClientFactory

[py.warnings] WARNING: /.../scrapy/core/downloader/contextfactory.py:51: DeprecationWarning: Passing method to twisted.internet.ssl.CertificateOptions was deprecated in Twisted 17.1.0. Please use a combination of insecurelyLowerMinimumTo, raiseMinimumTo, and lowerMaximumSecurityTo instead, as Twisted will correctly configure the method.
acceptableCiphers=DEFAULT_CIPHERS)

@kmike
Copy link
Member

kmike commented Jun 28, 2018

How did you get these warning?

@hermit-crab
Copy link

hermit-crab commented Jul 31, 2018

I've recently also started getting these and a few other warnings but it turned out to be one of the spiders in the project was importing a certain library which tinkered with the warnings.simplefilter and such. Was a bit of a headache to track it down. I'd check all foreign imports first if I were you @Aqua-Dream

@Aqua-Dream
Copy link
Author

Aqua-Dream commented Aug 2, 2018

Thanks @Unknowny
I also found out that python will not display DeprecationWarning by default.

@Gallaecio
Copy link
Member

Gallaecio commented Jan 24, 2019

@kmike I can reproduce the first warning (made visible by pytest):

venv/lib/python2.7/site-packages/scrapy/core/downloader/webclient.py:4
  /home/adrian/scrapinghub/veritec-solutions-crawler/venv/lib/python2.7/site-packages/scrapy/core/downloader/webclient.py:4: DeprecationWarning: twisted.web.client.HTTPClientFactory was deprecated in Twisted 16.7.0: please use https://pypi.org/project/treq/ or twisted.web.client.Agent instead
    from twisted.web.client import HTTPClientFactory

We are indeed using HTTPClientFactory: https://github.com/scrapy/scrapy/blob/master/scrapy/core/downloader/webclient.py#L4

Should we reopen this issue?

@elacuesta
Copy link
Member

elacuesta commented Jul 17, 2019

I find this somewhat frustrating. I could not find any release notes for v16.7.0 talking about this deprecation, in fact there is no mention of "16.7.0" in https://github.com/twisted/twisted/blob/trunk/NEWS.rst and https://twistedmatrix.com/documents/current/api/twisted.web.client.HTTPClientFactory.html says nothing about it being deprecated.
And the warning just says "X was deprecated: please use Y or Z", which doesn't provide much insight as the migration is not straightforward, from what I could see they are different classes with different interfaces 🤷‍♂️

@Gallaecio
Copy link
Member

Gallaecio commented Jul 17, 2019

I believe this is the point before the last of the deprecation list of Twisted Core 17.1.0. It’s easy to miss because it does not mention the factory at all.

You can see the 8960 ticket in their issue tracking system for additional information, although the actual pull request, twisted/twisted#643, may be more useful.

@wRAR
Copy link
Contributor

wRAR commented Nov 19, 2019

So I've read the links above and I still don't understand if the problems mentioned there are applicable to Scrapy. Should we plan to replace this code with the modern one?

@Gallaecio
Copy link
Member

Gallaecio commented Nov 19, 2019

That would be great, in my opinion, much cleaner than #4165.

However, there’s a risk of introducing changes of behavior. And there’s the argument that HTTP 1.0 usage is dropping, so if such a change requires a lot of effort, it may not be worth it.

@nyov
Copy link
Contributor

nyov commented Feb 28, 2020

But the "replacement code" is the HTTP1.1 downloader already, IMO.
Seeing how the old downloader here also doesn't support HTTPS, I only see it used in some special cases (maybe comparable to the FTP downloader), and just keeping the current behavior around for those cases feels reasonable to me. So I approve #4165.
The only change is pulling the rest of this single Twisted subclass into scrapy, from a high-level standpoint.

Edit: Sorry, I got this wrong, tests say the http10 handler does handle HTTPS just fine, even does TLS SNI, it seems. So that's a bogus argument from me.
(So I'm not sure now if there is any reasonable difference in behavior to the new handler, or if it's just another codepath for the same result, that we could simply deprecate.)

@Gallaecio
Copy link
Member

Gallaecio commented Aug 12, 2022

The second warning is still an issue. Offending line:

method=getattr(self, 'method', getattr(self, '_ssl_method', None)),

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

No branches or pull requests

7 participants