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

Default reactor to None, which makes class Server compatibile with old… #1044

Conversation

barry-scott-forcepoint
Copy link

…er version of twisted,

like 2.0.

https://twistedmatrix.com/trac/ticket/9503#ticket

Copy link
Contributor

@wiml wiml left a comment

Choose a reason for hiding this comment

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

This seems reasonable to me as far as it goes (though the same change should probably be made to twisted.internet.unix.Server as well; they both gained the extra argument in the same commit).

However, the comment at the top of tcp.py suggests that these interfaces should be thought of as kind of internal (which is presumably why an incompatible change was made). Why do you need to invoke Server(...) directly? Is there an enhancement that could be made to a higher level, more-public API instead?

@barry-scott-forcepoint
Copy link
Author

We are a MITM engine. The high level APIs do not allow us to get at the details of the HTTP requests that is necessary to build our application.

@glyph
Copy link
Member

glyph commented Dec 30, 2018

We are a MITM engine. The high level APIs do not allow us to get at the details of the HTTP requests that is necessary to build our application.

Hi @barry-scott-forcepoint,

In a general sense, I understand that you might need some deeper integration than might be available via the public API. But, more specifically to tcp.py, what parameters do you need that aren't exposed publicly?

@glyph
Copy link
Member

glyph commented Dec 30, 2018

(My personal feeling on this is that compatibility with Twisted 2.0, in APIs that specifically describe themselves as "internal", is an explicit non-goal. We will fix stuff like this pre-release, since technically tcp.Server is in a "public" namespace with respect to our compatibility policy, but this change has been out there way too long to revert on its own. I really hope that we have the bandwidth sometime soon to simply underscore-prefix tcp.py, udp.py, and friends.)

@barry-scott-forcepoint
Copy link
Author

I no longer need this patch merged. We refactored the code that needed this.

@glyph
Copy link
Member

glyph commented Jan 4, 2019

I no longer need this patch merged. We refactored the code that needed this.

Thanks for letting us know. Hopefully using new public interfaces! I am sorry that Twisted didn't provide a great experience in this regard, but thanks for your contribution nonetheless.

@glyph glyph closed this Jan 4, 2019
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.

None yet

3 participants