Add listening/close events, and a close() function

[timoxley]

Irritating using xmlrpc in a test suite as there's no easy way to stop the server from listening on a port. This patch forwards the http server's 'listening' and 'close' events, and exposes a close() function on the server.
This would allow your own server tests to not require use of unreliable stuff like setTimeout(doStuff, 500) to "ensure" server is listening.

One tricky issue that needs to be resolved though, is the fact that normal rpc calls are named events, and this could create confusion for anyone who happens to want to call functions named 'close' or 'listening'.

Three possible solutions:

• Option 1: Namespace/private the event names (e.g. "xmlrpc/close" or "_close") to make them far less likely to create conflicts.
• Option 2: Expose the http server and listen for events directly on it, e.g

var server = xmlrpc.createServer(options)
server.httpServer.on('listening', onListening)

• Option 3: Change api to something like:

var server = xmlrpc.createServer().listen(options, onListening)
server.close(onClose)

Last solution probably the best but also most disruptive, second solution good middle ground but verbose, first solution easiest, but doesn't fully solve the problem. Let me know which option sounds good to you and it will be done.

[travisbot]

This pull request fails (merged d71f0ae into cd4c57c).

[timoxley]

…and apparently the PR needs fixing for node 0.6 and 0.4… brb

[travisbot]

This pull request passes (merged 743cc62 into cd4c57c).

[baalexander]

Thanks @timoxley for the pull request (and updating the tests to pass). I'll have time tomorrow to review this (and the other pending pull requests) and comment/merge.

[baalexander]

I agree that Option 3 would be best, but enforcing .listen() would require too much change for a 1.x release.

What about a combination of Option 2 and Option 3? We could change var httpServer to this.httpServer, exposing server.httpServer.on('listening', onListening) and provide a .close() function on Server that would just forward to httpServer (or they could call this.httpServer.close() directly.

Thoughts?

[timoxley]

The hybrid concept sounds reasonable. Agree, major breaking changes should definitely go in a 2.x.

[timoxley]

The underlying problem though, probably, is (mis)using events to drive the procedure calls, which removes the ability to use events for any asynchronous things that can happen with a http connection, plus it's also a little strange IMO. Ideally, the remote interface would simply be an object representing the available API, and it uses something like the following to actually invoke the api:

//pseudo code
remote.on('methodCall', function(method, args) {
  server[method].apply(server, args);
});

Now I think about it, this is exactly how I abstracted your api in my own app though this is really a discussion for post 1.x.

[timoxley]

One other option would be to provide an optional 'on listening' callback to the constructor

[timoxley]

or a .ready function that would be called when the server is ready

[baalexander]

First, sorry for my delayed responses. I'm in the process of moving to San Francisco and that's taking up most of my time.

I added an issue to discuss the 'methodCall' event. While we both agree it probably belongs in a 2.x, I didn't want to forget about it. And this way others can discuss later if they want to. I think the move could be good, but I do admit I like the convenience of .on('methodname').

Is the reason for adding a .ready() or an onListening callback in the constructor over server.httpServer.on('listening', onListening) related to timing issues (could start listening before the request to httpServer.on) or convenience? I'm liking the listener callback in the constructor being similar to Node's HTTP.

I'll be happy to implement the hybrid approach + listener in constructor (or a .ready) if you'd like, but a heads up that it may be after the move.

[timoxley]

Gah, sorry for the delay, I went on holiday. :D

The reason for .ready/constructor is exactly that. A .ready doesn't 100% solve the issue really. Now, if the server.httpServer.listen(port) command is inside of a nextTick (which it is), so long as the user adds the listener in the same tick, they should be fine, but requiring the user to be aware of which tick they are in could potentially create bugs, though this is a common problem.

I'll put the listener in the constructor, I think that's the most concise of the options. PR incoming.

[travisbot]

This pull request passes (merged 0c3de93 into cd4c57c).