Opened 6 years ago

Closed 6 years ago

#3443 defect closed fixed (fixed)

twisted.conch.ssh.factory.SSHFactory.buildProtocol doesn't let you customize the protocol it creates

Reported by: mwh Owned by:
Priority: normal Milestone:
Component: conch Keywords: review
Cc: Branch: branches/ssh-factory-build-protocol-3443
(diff, github, buildbot, log)
Author: jml Launchpad Bug:

Description

We want to set tcp keepalives on all connections to our ssh server, which would seem to be most logically done in connectionMade of the protocol object created on a connection -- but there's no way to influence the class of the protocol object created here, other than copy-paste-hacking buildProtocol into your factory subclass.

If SSHFactory.buildProtocol delegated to Factory.buildProtocol, you could do it by defining 'protocol' in your factory subclass, as in most of the rest of Twisted.

Diff attached, no tests though.

Attachments (4)

Change History (18)

Changed 6 years ago by mwh

comment:1 Changed 6 years ago by mwh

  • Component changed from core to conch
  • Owner changed from glyph to z3p
  • Type changed from enhancement to defect

comment:2 Changed 6 years ago by mwh

  • Keywords review added

comment:3 Changed 6 years ago by mwh

  • Owner z3p deleted

comment:4 Changed 6 years ago by jml

  • Keywords review removed
  • Owner set to mwh

If you want to test this (and you do), then probably the best thing to do is add a 'test_factory' file to t/conch/tests, and then demonstrate that calls to buildProtocol call self.protocol with the correct parameters.

It might even be ok to just check that the return value of buildProtocol is an instance of self.protocol.

comment:5 Changed 6 years ago by mwh

  • Keywords review added
  • Owner mwh deleted

How does this look?

comment:6 Changed 6 years ago by jml

  • Keywords review removed
  • Owner set to mwh
  • Need two lines of vertical whitespace between methods
  • Use docstrings rather than comments for tests: """\nFoo.\n"""
  • Please add a docstring to testMultipleFactories.

Thanks so much.

comment:7 Changed 6 years ago by mwh

  • Keywords review added
  • Owner mwh deleted

Now with less Launchpad coding style.

comment:8 Changed 6 years ago by jml

  • Keywords review removed
  • Owner set to jml

OK. This is approved to land.

comment:9 Changed 6 years ago by exarkun

testBuildProtocol and testBuildProtocolRespectsProtocol are misnamed

comment:10 Changed 6 years ago by jml

  • Owner changed from jml to mwh

Oops, missed that.

Specifically, they should be test_buildProtocol and test_buildProtocolRespectsProtocol.

comment:11 Changed 6 years ago by mwh

  • Keywords review added
  • Owner mwh deleted

Like so?

comment:12 Changed 6 years ago by jml

  • author set to jml
  • Branch set to branches/ssh-factory-build-protocol-3443

(In [24899]) Branching to 'ssh-factory-build-protocol-3443'

comment:13 Changed 6 years ago by jml

  • Resolution set to fixed
  • Status changed from new to closed

(In [24901]) Make SSHFactory use self.protocol to construct protocol instances.

  • Author: mwh
  • Reviewer: jml
  • Fixes #3443

Previously SSHFactory always instantiated an SSHServerTransport. This
commit allows subclasses of SSHFactory to specify their own protocol --
usually a subclass of SSHServerTransport.

comment:14 Changed 3 years ago by <automation>

Note: See TracTickets for help on using tickets.