httpd.js needs a port property

RESOLVED INVALID

Status

Testing
httpd.js
RESOLVED INVALID
5 years ago
5 years ago

People

(Reporter: mihneadb, Assigned: mihneadb)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

5 years ago
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.
(Assignee)

Comment 1

5 years ago
Created attachment 767419 [details] [diff] [review]
add the port property
Assignee: nobody → mihneadb
Attachment #767419 - Flags: review?(jwalden+bmo)
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
 */
Attachment #767419 - Flags: review?(jwalden+bmo) → feedback+
(Assignee)

Updated

5 years ago
Blocks: 887054
(Assignee)

Updated

5 years ago
Blocks: 887064
(Assignee)

Updated

5 years ago
Blocks: 887480
FWIW, you can already get the port the server is running on from:

server.identity.primaryPort
(Assignee)

Comment 4

5 years ago
Thanks! So then should I just drop this?
(Assignee)

Updated

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

Yeah, I think so.
(Assignee)

Comment 6

5 years ago
(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.
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.
(Assignee)

Comment 8

5 years ago
My purpose was to get the port that a client should connect to. So the one visible 'externally'.
(Assignee)

Updated

5 years ago
Blocks: 887578
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.
(Assignee)

Comment 10

5 years ago
Ok, then I'm closing this bug. Thanks for the pointers!
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → INVALID
(Assignee)

Updated

5 years ago
No longer blocks: 887557
(Assignee)

Updated

5 years ago
No longer blocks: 887578
(Assignee)

Updated

5 years ago
No longer blocks: 887480
(Assignee)

Updated

5 years ago
No longer blocks: 887064
(Assignee)

Updated

5 years ago
No longer blocks: 887054
You need to log in before you can comment on or make changes to this bug.