886980 - httpd.js needs a port property

Status: RESOLVED INVALID

(Testing :: General, defect)

Description

[Mihnea Dobrescu-Balaur (:mihneadb)]

httpd.js accepts -1 as a port and it will choose an available port. After that, we need to find out what port the server runs on. Adding this property so we don't have to use _port.

Comment 1

[Mihnea Dobrescu-Balaur (:mihneadb)]

Attachment: add the port property

Comment 2

[Jeff Walden [:Waldo]]

Comment on attachment 767419 [details] [diff] [review]
add the port property

Review of attachment 767419 [details] [diff] [review]:
-----------------------------------------------------------------

Seems sensible enough.  I'd like this made a little more robust, tho, before giving it an r+.  See the comments.

Also, this needs a test.  I'd suggest adding in some tests of this in netwerk/test/httpserver/test/test_start_stop.js -- one before the server is ever started, that checks the right error is thrown (follow the pattern used in that file); one after the server is started, that checks for the correct value; and one after the server is shut down, that checks for the right error being thrown again.

::: netwerk/test/httpserver/httpd.js
@@ +642,5 @@
>      this._handler.registerContentType(ext, type);
>    },
>  
>    //
> +  // returns the port the server runs on

Make this // see nsIHttpServer.port instead.

@@ +646,5 @@
> +  // returns the port the server runs on
> +  //
> +  get port()
> +  {
> +    return this._port;

If the server isn't running, this will return the last port it was running on, which seems confusing and/or unhelpful.  Let's make this throw an exception when the server's not running by adding this at the start:

if (!this._socket)
  throw Cr.NS_ERROR_UNEXPECTED;

::: netwerk/test/httpserver/nsIHttpServer.idl
@@ +177,5 @@
>     *   handler, under the key "directory".
>     */
>    void setIndexHandler(in nsIHttpRequestHandler handler);
>  
> +  /** Represesnts the port that the server runs on. **/

/**
 * Represents the port that the server is currently running on.
 *
 * @throws NS_ERROR_UNEXPECTED
 *   if the this server is not currently running
 */

Comment 3

[Nathan Froyd [:froydnj]]

FWIW, you can already get the port the server is running on from:

server.identity.primaryPort

Comment 4

[Mihnea Dobrescu-Balaur (:mihneadb)]

Thanks! So then should I just drop this?

Comment 5

[Nathan Froyd [:froydnj]]

(In reply to Mihnea Dobrescu-Balaur (:mihneadb) from comment #4)
> Thanks! So then should I just drop this?

Yeah, I think so.

Comment 6

[Mihnea Dobrescu-Balaur (:mihneadb)]

(In reply to Nathan Froyd (:froydnj) from comment #5)
> (In reply to Mihnea Dobrescu-Balaur (:mihneadb) from comment #4)
> > Thanks! So then should I just drop this?
> 
> Yeah, I think so.

:waldo, do you agree? Maybe at least we could document this better.

Comment 7

[Jeff Walden [:Waldo]]

Good call on pointing out the identity stuff, it's been too long since I worked on any of this much.

It's not always the case that primaryPort and the socket's port will be the same.  The identity mechanism describes the locations the server purports to be -- there's no inherent connection between those locations and the actual port in use.  If the user wanted to run his server on port 12345, but he wants the server to act as if it were at localhost:54321, he can do that.  (A browser connecting to the server has to know to contact it via port 12345, not 54321, but that's what PAC is for.  Or even just direct connections hand-written out via the telnet command or similar.)

So the question is: what's the purpose of the port number being retrieved?  Is it supposed to be the underlying physical port used on the machine?  Or is it supposed to be the visible location the server thinks it's at?  It seems to me that these are two distinct concepts, likely deserving two distinct places where they're exposed.

Comment 8

[Mihnea Dobrescu-Balaur (:mihneadb)]

My purpose was to get the port that a client should connect to. So the one visible 'externally'.

Comment 9

[Jeff Walden [:Waldo]]

Sounds like you want server.identity.primaryPort, then.  If someone wants the actual physical port -- and I suspect they will, eventually; it'd show up in places like the PAC URL generated in build/automation.py.in -- then we can add something for it then.

Comment 10

[Mihnea Dobrescu-Balaur (:mihneadb)]

Ok, then I'm closing this bug. Thanks for the pointers!