Changed --listen parsing to use Net::Server::Proto->parse_info()

[kriegfrj]

Per the title, I ran into a situation where I wanted to use my own custom implementation of Net::Server::Proto::SSL that allowed me to hook into the handshake process. The standard Net::Server:Proto::parse_info method supports port specifications in string format that allow you to specify your own custom protocol package, eg in a form like this:

1.2.3.4:80/My::Custom::Proto

See perldoc for Net::Server::Proto for more examples.

I thought it would be good to have this same flexibility in the --listen option for Starman, which was a half-way solution that duplicated a subset of the functionality that was already in Net::Server::Proto. Hence this PR.

The only disadvantage of this PR that I can see is that the syntax for specifying Unix-domain sockets is slightly different:

# Old style
--listen /tmp/starman.sock
# New style
--listen /tmp/starman.sock|unix

So the price of the more efficient code is a slight loss of backward compatibility. It would probably be possible to add the new functionality while maintaining backwards compatibility, but this will make the code slightly more complicated again. If the maintainers of this package decide that backwards compatibility is important enough to try and keep, I can re-introduce it and re-submit a new pull request. Otherwise I'm happy for it to be used as-is.

Thoughts?

[miyagawa]

I haven't looked at the code, but:

It would probably be possible to add the new functionality while maintaining backwards compatibility

The compatibility is important because it is a standard in plackup(1) (see perldoc plackup) so that you can use the same command line arguments for handlers that support plackup via Plack::Handler.

Like #111, you should probably add a new Starman::Server options to implement this feature, while adding shims in Plack::Handler::Starman to keep the compatibility if necessary.

[kriegfrj]

Thanks for the feedback. Understanding the rationale for the existing syntax helps.

In light of what you've said I've been having a bit of a think about this, and here's what I've come up with (feel free to correct me if you think I've got anything wrong). I think the fundamental problem here is that we have two incompatible command-line syntaxes - Net::Server syntax and Plack syntax - which are fighting each other. Because Starman::Server is based on Net::Server, its "native" language (if you like) is N::S, so to get Plack compatibility requires converting from plackup syntax to N::S syntax. Where I think we are getting ourselves in a knot is that the compatibility layer (the "shim" as you call it) which translates from plackup syntax to N::S syntax is at the moment (with the exception of that which was added in #111) is mostly embedded in Starman::Server::run. The problem with this is that the N::S syntax is hidden from all end users of Starman::Server (even if you subclass it directly). I think the move in #111 to do the shimming in Plack::Handler::Starman was a good idea, and I think it would be even better if the existing shim code in Starman::Server was moved there too. If this were done, then I would have direct access to the N::S-style syntax for port directly by subclassing Starman::Server or by invoking it from my own custom script - at the moment this syntax is hidden because of the Plack compatibility code embedded in Starman::Server::run. Does this make sense?

The main problem with this approach is that it would break compatibility for anyone using Starman::Server directly instead of through Plack::Handler::Starman. I think it would also be possible to work around this if backwards compatibility is important for this, as I am not sure how common it is for people to use Starman::Server directly in this way.

Thoughts?

[miyagawa]

I think the fundamental problem here is that we have two incompatible command-line syntaxes - Net::Server syntax and Plack syntax - which are fighting each other.

Correct.

The problem with this is that the N::S syntax is hidden from all end users of Starman::Server (even if you subclass it directly).

I'm not sure if that's actually a problem. Starman using Net::Server has been just an implementation detail, and the fact we added #111 is that we're now finally opening it up for an end user, although that code still hasn't hit CPAN yet. I still have a slight doubt whether it was a right move and there's a chance still I'd call the feature experimental.

I think it would be even better if the existing shim code in Starman::Server was moved there too.

Right, that makes sense if we decide to expose more (most) of Net::Server features directly.

I think it would also be possible to work around this if backwards compatibility is important for this, as I am not sure how common it is for people to use Starman::Server directly in this way.

It might not be so common, but I think the compatibility is important. Since we're adding the "direct Net::Server compatibility" in #111 and this issue, to me it rather makes more sense to implement them as a new options, or rather a new sub(or super)class that is closer to the metal of Net::Server.

[kriegfrj]

I'm not sure if that's actually a problem. Starman using Net::Server has been just an implementation detail, and the fact we added #111 is that we're now finally opening it up for an end user, although that code still hasn't hit CPAN yet. I still have a slight doubt whether it was a right move and there's a chance still I'd call the feature experimental.

Sorry, perhaps "problem" was not the correct word. It is not a problem if the focus for Starman is as a Plack implementation, as it has been historically. It's only a "problem" if we want to be able to support both Plack and Net::Server syntax simultaneously.

Btw, after you pointed me to #111 I had a quick attempt to try and do what I want to do by specifying my ports using the --net-server-port option rather than --listen. It sort-of worked but I still hit a couple of issues - namely, that Server::Starter was still adding a default port because it didn't find any --listen directives, and also that the current --net-server-xxx implementation doesn't support multi-valued arguments (so I'm limited to only one port). Again, these are things that I could probably work around, but it got me thinking why bother trying to find workarounds to re-introduce functionality that is already there under the abstraction layer? Seems unnecessarily complicated and an additional maintenance burden.

It might not be so common, but I think the compatibility is important. Since we're adding the "direct Net::Server compatibility" in #111 and this issue, to me it rather makes more sense to implement them as a new options, or rather a new sub(or super)class that is closer to the metal of Net::Server.

Understood. The only problem with maintaining backward compatibility is the extra maintenance overhead that it introduces (which my experience tells me should not be dismissed lightly). If the feature is not widely used, then it is legitimate to ask whether or not the extra maintenance overhead is justified, or whether it would be more efficient to ask those people using the deprecated syntax to adapt to the new syntax. Obviously this is a call only the package maintainer can make, and as you have expressed a desire to maintain backward compatibility I can only respect that decision.

I'm leaning towards the superclass option myself, as it seems to make more sense (and take less code) to expose the Net::Server functionality direct-from-the-metal rather than have a layer hiding the metal away and then another layer on top that tries to un-hide it. I'll try and come up with something later today and submit another implementation for your comments & feedback.

[miyagawa]

... Server::Starter was still adding a default port because it didn't find any --listen directives, and also that the current --net-server-xxx implementation doesn't support multi-valued arguments (so I'm limited to only one port)

The command line argument --net_server-xxx= has such limitations, but the net_server_args argument passed directly to Starman::Server will bypass such mangling, so you can pass whatever data structure to it, can't you?

I'm not suggesting that the net_server_args is the best option to accomplish what you do, but just wanted to point out that there's a way to work around that today.

[kriegfrj]

Thanks for the tip. Per your suggestion, I managed to get it working by writing my own startup script which instantiated Starman::Server directly and passed in "port" directives via the net_server_args hash. Instantiating Starman::Server directly also bypassed the addition of the default port 5000 (which was being added by Plack::Runner).

One barrier I did run into is that while the Net::Server subclasses accept port directives in either the string format or the pre-parsed hash format; however there is some code in Starman::Server which assumes they will be in the hash format (eg, { port => 80 } rather than ":80"). So I needed to make sure the "port" directives I used in the net_server_args hash used the hash format. This was fairly easy to work around.

This works out-of-the-box from master, so I think that makes this PR redundant to some extent. I'll close it for now and re-open if I can see a need for it in the future.

Thanks again for your help in coming to a solution.