[
https://issues.apache.org/jira/browse/THRIFT-2398?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Randy Abernethy updated THRIFT-2398:
------------------------------------
Attachment: 0001-nodejs-server-and-option-cleanup.patch
This patch reduces the node.js createServer() signature count to 3:
var server = thrift.createServer(processor, handler, options);
var server = thrift.createMultiplexServer(processor, options);
var server = thrift.createWebServer(options);
This eliminates a lot of redundant code and simplifies the Apache Thrift
Node.js UI quite a bit, also adds jsdoc documentation for all of the exported
interfaces. I think this is a worthy improvement but still leaves room for
discussion of a more Thrift like stack (TSocket, etc.) addition.
There is a single ServerOptions object shared by all three create functions.
The ServerOptions object has a tls attribute which if present causes all three
create functions to add ssl to the underlying transport. The tls object is not
used by the Apache Thrift lib but passed directly to Node.js, allowing any
legal Node.js tls options to be used (key and cert at a minimum).
I had not used the multiplexer in JavaScript until today. I found the tests not
working. They passed but the client side was not actually working (connect is
never invoking the processor). I removed the dedicated multiplexer test and
replaced it with the shared thrift/lib/nodejs/test/test_driver.js. These were
essentially identical except that the prior declared success after a 3 second
wait regardless. I cloned the master to make sure, but it is definitely broken
at the current HEAD. I'll create a separate ticket for this.
This patch also includes assorted minor code cleanup and comments. The node.js
and js tests all ran clean for me post this patch (with the mux exception
mentioned).
> Improve Node Server Library
> ---------------------------
>
> Key: THRIFT-2398
> URL: https://issues.apache.org/jira/browse/THRIFT-2398
> Project: Thrift
> Issue Type: Improvement
> Components: Node.js - Library
> Affects Versions: 0.9.2
> Environment: all
> Reporter: Randy Abernethy
> Attachments: 0001-nodejs-server-and-option-cleanup.patch
>
>
> Improve Node Server Library
> =======================
> Background
> ----------------
> In the last 7 months Node.js has gone from one server constructor:
> • createServer()
> to seven, adding:
> • createSSLServer()
> • createMultiplexServer()
> • createMultiplexSSLServer()
> • createHttpServer()
> • createHttpsSSLServer()
> • createWebServer()
> there are really only two implementations:
> • createMultiplexSSLServer()
> • createWebServer()
> Several of these servers have undocumented options and share options objects
> with other libraries.
> Proposal
> -------------
> 1. Remove all of the create signatures except these three:
> o createServer()
> o createMultiplexServer()
> o createWebServer()
> with createServer() implemented by createMultiplexServer(). All signatures
> will support optional multiplexing and optional SSL/TLS. Eliminate no present
> functionality and maintain signature compatibility in the three signatures
> preserved.
> 2. Document and standardize all server options and parameters with notes
> describing any deprecated features being preserved for backward compatibility.
> Motivation
> -------------
> The dispersion of create methods makes it harder for developers to know which
> server to use and leads to diffusion in the way that options and features are
> provided. This also complicates testing. Reducing the servers to the two
> currently supported end point transports (TCP and HTTP) will enforce
> standardization and simplify adoption. Now is the time to address these
> issues before the new create signatures show up in a released version.
> Approach to Options
> ----------------------------
> Presently the non-web server options objects may have transport and protocol
> properties. Undocumented key and cert properties are used to enable the
> options object to be passed to the Node.js tls and https createServer()
> methods. This approach requires Apache Thrift options to be visible to the
> Node.js library methods and vice versa. A better approach might be to place
> Node.js tls options in a tlsOptions object which is itself a property of the
> server options object. This will allow any tlsOptions to be passed through to
> Node.js without concerns for conflicts on the Node.js or Apache Thrift side,
> now or in the future. The presence of a tlsOptions object can also be used to
> enable SSL/TLS in the two server implementations rather than having separate
> functions.
> Would like to know what others think about this.
--
This message was sent by Atlassian JIRA
(v6.2#6252)
