On 02/20/2015 03:04 AM, Arne Bab. wrote: > Hi, > > The review of the FCP rewrite is a huge undertaking, so I’ll limit > myself to the functional changes since the review of bertm. > > To get only these, I filtered the commits by their commit messages[1] > and removed anything which looked like a documentation change or > purely stylistic changes. > > As a result the number of line changes (removed + added) went down > from 13834 to 1211.
This is a reduction to about 9% of the length and it's still absolutely
huge. Thank you for doing that.
> As first step I’ll purely paste the diff here (and attach it along
> with a htmlized version for people without diff highlighting).
>
> @xor: Please have a look at the commits and check whether any important
> functional change is missing (if yes, please add it). Also please give
> us a 6-line description of the comments from bertm in his review. We
> must be able to verify against that, and each one of us clicking each
> outdated comment on github is not a viable way to do that.
>
> Best wishes,
> Arne
>
> [1]: hg log --template "{node} : {desc|firstline}\n" -r
> "6c438b4c279d0a671be6516cee34052c7c3d5101::29ba0908ee2a81afdfaa1ab369382e1ad892b726"
> | sed "s/.*JavaDoc.*//" | sed "s/.*TODO.*//" | sed "s/.*Organize.*//" | sed
> s/.*ocument.*// | sed s/.*explain.*// | sed s/.*spelling.*// | sed
> s/.*sort.*// | sed s/.*[Ii]ndent.*// | sed s/.*[Mm]erge.*// | sed
> s/.*[Mm]ove.*// | sed s/.*[Ee]xtract.*// | sed s/.*[Rr]ename.*// | cut -d :
> -f 1 | xargs hg export
We've discussed on IRC that while this collection of commits is helpful
for incremental reviewing while a change is under development, it makes
changes very difficult to review as a whole. Things are added and then
modified (and in some cases even removed) instead of just introduced in
their final form. In the future please open pull requests for things
like this with a commit containing each high-level change in the overall
diff. Part of submitting a changeset for review is making it easy to review.
> # HG changeset patch
> # User xor-freenet <x...@freenetproject.org>
> # Date 1420557120 -3600
> # Tue Jan 06 16:12:00 2015 +0100
> # Node ID 6c438b4c279d0a671be6516cee34052c7c3d5101
> # Parent 754640ea02b7a7a8b2165aa759c5eb10eb02eff0
> FCPPluginClient.send(): Improve readability
>
> - Improve documentation for the codepath of delivering messages to
> sendSynchronous() threads.
> - Rename maybeDispatchMessageLocallyToSendSynchronousThread() to
> dispatchMessageLocallyToSendSynchronousThreadIfExisting(). Thats more
> uniform with the other dispatch functions since they now all are
> prefixed with "dispatch" , and the "IfExisting" is more
> self-explanatory than "maybe" because it implies the condition upon
> which it is done: If a sendSynchronous() thread exists.
>
> diff -r 754640ea02b7 -r 6c438b4c279d
> src/freenet/clients/fcp/FCPPluginClient.java
...
> - if(maybeDispatchMessageLocallyToSendSynchronousThread(direction,
> message))
> + // The message handler is determined to be local at this point.
> There are two possible
> + // types of local message handlers:
> + // 1) An object provided by the server/client which implements
> FredPluginFCPMessageHandler.
> + // The message is delivered by executing a callback on that
> object, with the message
> + // as parameter.
> + // 2) A call to sendSynchronous() which is blocking because it is
> waiting for a reply
> + // to the message it sent so it can return the reply message to
> the caller.
> + // The reply message is delivered by passing it to the
> sendSynchronous() thread through
> + // an internal table of this class.
> + //
> + // The following function call checks for whether case 2 applies,
> and handles it if yes:
> + // If there is such a waiting sendSynchronous() thread, it delivers
> the message to it, and
> + // returns true, and we are done: By contract, messages are
> preferably delivered to
> + // sendSynchronous().
> + // If there was no sendSynchronous() thread, it returns false, and
> we must continue to
> + // handle case 1.
> +
> if(dispatchMessageLocallyToSendSynchronousThreadIfExisting(direction,
> message))
> return;
>
> // We now know that the message handler is not attached by network,
> and that it is not a
While this setup may work, it seems like there's an alternative to the
introductory paragraph. Something like
if (sentSynchronously(message))
sendSynchronousReply(message);
else
sendReply(message);
This doesn't block merging, but I'd like to consider this as a possible
future simplification that has the same overall flow. (Why doesn't the
message itself contain/determine both the type and the direction and
behave appropriately instead?)
...
> # HG changeset patch
> # User xor-freenet <x...@freenetproject.org>
> # Date 1420559434 -3600
> # Tue Jan 06 16:50:34 2015 +0100
> # Node ID 53db2d8eab097abbb354db7f4ed641362addaecb
> # Parent 6c438b4c279d0a671be6516cee34052c7c3d5101
> FCPConnectionHandler: Add send() to replace public member variable usage
>
> - Temporarily deprecate the function of the member variable which led to
> public usage of it.
> - Add documentation about the deprecation to tell people to replace it
> with send().
> - Also document the temporary character of the deprecation.
>
> diff -r 6c438b4c279d -r 53db2d8eab09
> src/freenet/clients/fcp/FCPConnectionHandler.java
...
> + /**
> + * Queues the message for sending at the {@link
> FCPConnectionOutputHandler}.<br>
> + * <br>
> + *
> + * ATTENTION: The function will return immediately before even trying to
> send the message, the
> + * message will be sent asynchronously.<br>
> + * As a consequence, this function not throwing does not give any
> guarantee whatsoever that the
> + * message will ever be sent.
That this comment is necessary suggests there should be a send function
with more guarantees. (Is this provided by the synchronous sending?)
> + */
> + @SuppressWarnings("deprecation")
> + public final void send(final FCPMessage message) {
> + outputHandler.queue(message);
> + }
...
> # HG changeset patch
> # User xor-freenet <x...@freenetproject.org>
> # Date 1420835780 -3600
> # Fri Jan 09 21:36:20 2015 +0100
> # Node ID d932dec809fd97d223ae73c3f58b7c69f4750576
> # Parent a424a96cb813df6751f7dc5f389bb9643f854680
> FCPPluginClient.isServerDead(): Trim visibility
>
> diff -r a424a96cb813 -r d932dec809fd
> src/freenet/clients/fcp/FCPPluginClient.java
...
> /**
> + * ATTENTION: Only for internal use in {@link
> FCPConnectionHandler#getPluginClient(String)}.<br>
> + * Server / client code should always {@link #send(SendDirection,
> FCPPluginMessage)} messages
> + * to check whether the connection is alive. (To ensure that the
> implementation of this class
> + * could safely be changed to allow the server to be attached by network
> instead of always
> + * running locally in the same node as it currently is.)
That this comment exists means that package-private isn't sufficient,
and anything more restrictive is too much. This suggests to me that a
different class structure to make this message unnecessary would have
been preferable. I suppose this doesn't block merging either.
...
> # HG changeset patch
> # User xor-freenet <x...@freenetproject.org>
> # Date 1420837732 -3600
> # Fri Jan 09 22:08:52 2015 +0100
> # Node ID e255017cc021aae2ee7ff7a773b7ad37343d37ea
> # Parent d932dec809fd97d223ae73c3f58b7c69f4750576
> FCPPluginClient.getServerPluginName(): Trim visibility
>
> The JavaDoc already labeled it as "for internal use"
>
> diff -r d932dec809fd -r e255017cc021
> src/freenet/clients/fcp/FCPPluginClient.java
> --- a/src/freenet/clients/fcp/FCPPluginClient.java Fri Jan 09 21:36:20
> 2015 +0100
> +++ b/src/freenet/clients/fcp/FCPPluginClient.java Fri Jan 09 22:08:52
> 2015 +0100
> @@ -470,7 +470,7 @@
> * The class name of the plugin to which this FCPPluginClient is
> connected.
> * @see This is for internal usage by {@link
> FCPConnectionHandler#getPluginClient(String)}.
> */
> - public String getServerPluginName() {
> + String getServerPluginName() {
> return serverPluginName;
> }
We discussed how this function is later removed entirely, but is still
referenced in the serverPluginName documentation. (We also discussed how
this file is renamed to FCPPluginConnectionImpl.)
...
> # HG changeset patch
> # User xor-freenet <x...@freenetproject.org>
> # Date 1421260650 -3600
> # Wed Jan 14 19:37:30 2015 +0100
> # Node ID 788be9e7cba7f9710f0238170c73b80eb06153c2
> # Parent d3cef1a22d8cb721113847a80c6b280a900c7e72
> Fix missed "FCPPluginClient" -> "FCPPluginConnectionImpl" renaming
>
> The "Client" -> "Connection" interface extraction + renaming is not
> finished yet, as some places where usage of FCPPluginConnectionImpl
> has to be replaced with FCPPluginConnection might still remain.
> This merely fixes all remaining textual references of "FCPPluginClient"
>
> diff -r d3cef1a22d8c -r 788be9e7cba7
> src/freenet/clients/fcp/FCPPluginConnectionImpl.java
> --- a/src/freenet/clients/fcp/FCPPluginConnectionImpl.java Wed Jan 14
> 19:28:59 2015 +0100
> +++ b/src/freenet/clients/fcp/FCPPluginConnectionImpl.java Wed Jan 14
> 19:37:30 2015 +0100
> @@ -123,7 +123,7 @@
>
>
> /**
> - * Unique identifier among all FCPPluginClients.
> + * Unique identifier among all {@link FCPPluginConnection}s and
> FCPPluginConnectionImpls
FCPPluginConnectionImpls are FCPPluginConnections. I don't think this
should not reference the current implementation class of the interface.
Is it okay because this is FCPPluginConnectionImpl?
...
> # HG changeset patch
> # User xor-freenet <x...@freenetproject.org>
> # Date 1421336574 -3600
> # Thu Jan 15 16:42:54 2015 +0100
> # Node ID 15fef97e809064eac67f5a6b3da16d82d2a09abd
> # Parent 9280fa1593f0ccff425467fcdce7acc636a59fed
> FCPPluginConnectionImpl.sendSynchronous(): Unify error message format
>
> Also explain *what* timed out
>
> diff -r 9280fa1593f0 -r 15fef97e8090
> src/freenet/clients/fcp/FCPPluginConnectionImpl.java
> --- a/src/freenet/clients/fcp/FCPPluginConnectionImpl.java Wed Jan 14
> 22:40:19 2015 +0100
> +++ b/src/freenet/clients/fcp/FCPPluginConnectionImpl.java Thu Jan 15
> 16:42:54 2015 +0100
> @@ -850,9 +850,10 @@
> // Include the FCPPluginMessage in the Exception so the
> developer can determine
> // whether it is an issue of the remote side taking a
> long time to execute
> // for certain messages.
> - throw new IOException("The synchronous call timed out
> for " + this + "; "
> - + "direction = " + direction + "; "
> - + "message = " + message);
> + throw new IOException("sendSynchronous() timed out
> waiting for reply! "
I think an exception type for timeouts would be preferable? An
IOException can mean a lot of things. Is the thought that the reason
behind the failure doesn't change the possible recovery?
> + + " connection = " + FCPPluginConnectionImpl.this
> + + "; SendDirection = " + direction
> + + "; message = " + message);
> }
>
> // The thread which sets synchronousSend.reply to be
> non-null calls
...
> # HG changeset patch
> # User xor-freenet <x...@freenetproject.org>
> # Date 1421450378 -3600
> # Sat Jan 17 00:19:38 2015 +0100
> # Node ID ec7d03e9917e283ef16443ecaf86dea4ae2e04da
> # Parent 6e853b7efb00230b05545ec54b1074990934ebf3
> FCPPluginConnectionImpl: Add member class DefaultSendDirectionAdapter
>
> Will be used to implement the send funtions which default to a specific
> SendDirection.
>
> diff -r 6e853b7efb00 -r ec7d03e9917e
> src/freenet/clients/fcp/FCPPluginConnectionImpl.java
> --- a/src/freenet/clients/fcp/FCPPluginConnectionImpl.java Fri Jan 16
> 22:52:20 2015 +0100
> +++ b/src/freenet/clients/fcp/FCPPluginConnectionImpl.java Sat Jan 17
> 00:19:38 2015 +0100
> @@ -889,6 +889,69 @@
> }
> }
>
> + /**
> + * Encapsulates a FCPPluginConnectionImpl object and a default {@link
> SendDirection} to
> + * implement the send functions which don't require a direction
> parameter:<br>
> + * - {@link FCPPluginConnection#send(FCPPluginMessage)}<br>
> + * - {@link FCPPluginConnection#sendSynchronous(FCPPluginMessage,
> long)}}<br><br>
> + *
> + * An adapter is needed instead of storing this as a member variable in
> FCPPluginConnectionImpl
> + * because a single FCPPluginConnectionImpl object is handed out both to
> the the server AND the
> + * client which uses, and their default send direction will be
> different:<br>
> + * A server will want to send to the client by default, but the client
> will want to default to
> + * sending to the server.<br><br>
I don't understand why this *requires* adapters. (One could make the
case that it's nicer? I am not doing so here.) It sounds like a
constructor argument of "default direction" in FCPPluginConnectionImpl
is sufficient.
...
> # HG changeset patch
> # User xor-freenet <x...@freenetproject.org>
> # Date 1421684302 -3600
> # Mon Jan 19 17:18:22 2015 +0100
> # Node ID 15ce0a77c5cb872b617f7eede166df65f5b6b322
> # Parent a22003ad422630d1bf85a7dca52011540aa3204d
> FCPPluginConnectionImpl: Add member class SendToClientAdapter
>
> Implements DefaultSendDirectionAdapter with the direction of sending to
> the client.
> Not private since it will have to be used by the FCPServer. Explanation
> will follow when committing that code.
My question on why adapters are required continues.
...
> # HG changeset patch
> # User xor-freenet <x...@freenetproject.org>
> # Date 1421685975 -3600
> # Mon Jan 19 17:46:15 2015 +0100
> # Node ID 9bc0180a40977468419e9ae3c8ec729d3d6ea9ca
> # Parent 1e611fd6dbf22ef88c8fa0d0edc9ee3a2d01bb40
> FCPPluginConnectionImpl: Add storage for DefaultSendDirectionAdapters
>
> This also changes the factory functions
> FCPPluginConnectionImpl.construct*() to automatically register the
> connection at the FCPPluginConnectionTracker, which is more robust
> anyway.
>
> diff -r 1e611fd6dbf2 -r 9bc0180a4097
> src/freenet/clients/fcp/FCPPluginConnectionImpl.java
...
> * The client is not running within the node, it is attached by network
> with a
> * {@link FCPConnectionHandler}.<br/>
> *
> - * @see #constructForNetworkedFCP(Executor, PluginManager, String,
> FCPConnectionHandler)
> + * @see #constructForNetworkedFCP(FCPPluginConnectionTracker, Executor,
> PluginManager, String,
> + * FCPConnectionHandler)
> * The public interface to this constructor.
> */
> - private FCPPluginConnectionImpl(Executor executor, String
> serverPluginName,
> - ServerSideFCPMessageHandler serverPlugin, FCPConnectionHandler
> clientConnection) {
> + private FCPPluginConnectionImpl(FCPPluginConnectionTracker tracker,
> Executor executor,
> + String serverPluginName, ServerSideFCPMessageHandler
> serverPlugin,
> + FCPConnectionHandler clientConnection) {
>
> + assert(tracker != null);
> assert(executor != null);
> assert(serverPlugin != null);
> assert(serverPluginName != null);
Asserting? Why? I notice its (current) caller already asserts as well.
Is NPE not the expected behavior in that case instead? I agree that
failing early is preferable.
...
> * The client is not running within the node, it is attached by network
> with the given
> * {@link FCPConnectionHandler} clientConnection.<br/>
> *
> - * <p>You <b>must</b> register any newly created connections at
> - * {@link
> FCPPluginConnectionTracker#registerConnection(FCPPluginConnection)} before
> handing
> - * them out to plugin code.</p>
> + * The returned connection is registered at the given {@link
> FCPPluginConnectionTracker}.
> */
> - static FCPPluginConnectionImpl constructForNetworkedFCP(Executor
> executor,
> - PluginManager serverPluginManager, String serverPluginName,
> - FCPConnectionHandler clientConnection)
> + static FCPPluginConnectionImpl
> constructForNetworkedFCP(FCPPluginConnectionTracker tracker,
> + Executor executor, PluginManager serverPluginManager,
> + String serverPluginName, FCPConnectionHandler clientConnection)
> throws PluginNotFoundException {
>
> + assert(tracker != null);
> assert(executor != null);
> assert(serverPluginManager != null);
> assert(serverPluginName != null);
> assert(clientConnection != null);
Same question here too, and subsequent.
...
> diff -r 1e611fd6dbf2 -r 9bc0180a4097 src/freenet/clients/fcp/FCPServer.java
> --- a/src/freenet/clients/fcp/FCPServer.java Mon Jan 19 17:27:18 2015 +0100
> +++ b/src/freenet/clients/fcp/FCPServer.java Mon Jan 19 17:46:15 2015 +0100
> @@ -498,8 +498,10 @@
> throws PluginNotFoundException {
>
> FCPPluginConnectionImpl connection =
> FCPPluginConnectionImpl.constructForNetworkedFCP(
> - node.executor, node.pluginManager, serverPluginName,
> messageHandler);
> - pluginConnectionTracker.registerConnection(connection);
> + pluginConnectionTracker, node.executor, node.pluginManager,
> + serverPluginName, messageHandler);
> + // The constructor function already did this for us
> + /* pluginConnectionTracker.registerConnection(connection); */
Please remove this commented-out code.
> return connection;
> }
>
> @@ -531,8 +533,10 @@
> throws PluginNotFoundException {
>
> FCPPluginConnectionImpl connection =
> FCPPluginConnectionImpl.constructForIntraNodeFCP(
> - node.executor, node.pluginManager, serverPluginName,
> messageHandler);
> - pluginConnectionTracker.registerConnection(connection);
> + pluginConnectionTracker, node.executor, node.pluginManager,
> + serverPluginName, messageHandler);
> + // The constructor function already did this for us
> + /* pluginConnectionTracker.registerConnection(connection); */
> return connection;
> }
...
> # HG changeset patch
> # User xor-freenet <x...@freenetproject.org>
> # Date 1421688898 -3600
> # Mon Jan 19 18:34:58 2015 +0100
> # Node ID af4895a54495e22146f8fa45d06d1bd385c1f7bd
> # Parent 3c2ab5957f292e9dfe82e06e3fb08e742fcfe0da
> FCPPluginConnectionImpl: Add getDefaultSendDirectionAdapter()
>
> Uses the recently added table defaultSendDirectionAdapters to re-use
> existing adapters.
>
> diff -r 3c2ab5957f29 -r af4895a54495
> src/freenet/clients/fcp/FCPPluginConnectionImpl.java
> --- a/src/freenet/clients/fcp/FCPPluginConnectionImpl.java Mon Jan 19
> 18:28:22 2015 +0100
> +++ b/src/freenet/clients/fcp/FCPPluginConnectionImpl.java Mon Jan 19
> 18:34:58 2015 +0100
> @@ -1002,6 +1002,9 @@
> /** The {@link FCPPluginConnection#getID()} of the underlying
> connection. */
> private final UUID id;
>
> + /**
> + * Please use {@link
> FCPPluginConnectionImpl#getDefaultSendDirectionAdapter(SendDirection)}
> + * whenever possible to reuse adapters instead of creating new ones
> with this constructor.*/
Why? Time? Memory? Is there the danger that it will be stale? Is the
complication of making sure it's reinitialized worth it? (Did
benchmarking indicate it was impacting performance?)
...
> # HG changeset patch
> # User xor-freenet <x...@freenetproject.org>
> # Date 1421689122 -3600
> # Mon Jan 19 18:38:42 2015 +0100
> # Node ID bb09b50c03529e15e635c865d701236b8782bd69
> # Parent af4895a54495e22146f8fa45d06d1bd385c1f7bd
> FCPPluginConnectionImpl: Implement send functions without SendDirection
>
> The following commits will fix the code which hands out
> FCPPluginConnectionImpl to always wrap it inside a
> DefaultSendDirectionAdapter like the JavaDoc already promises.
>
> diff -r af4895a54495 -r bb09b50c0352
> src/freenet/clients/fcp/FCPPluginConnectionImpl.java
> --- a/src/freenet/clients/fcp/FCPPluginConnectionImpl.java Mon Jan 19
> 18:34:58 2015 +0100
> +++ b/src/freenet/clients/fcp/FCPPluginConnectionImpl.java Mon Jan 19
> 18:38:42 2015 +0100
> @@ -1067,6 +1067,44 @@
> return defaultSendDirectionAdapters.get(direction);
> }
>
> +
> + /**
> + * @throws NoSendDirectionSpecifiedException
> + * Is always thrown since this function is only implemented for
> FCPPluginConnectionImpl
> + * objects which are wrapped inside a {@link
> DefaultSendDirectionAdapter}.<br>
> + * Objects of type FCPPluginConnectionImpl will never be handed out
> directly to the server
> + * or client application code, they will always be wrapped in such
> an adapter - so this
> + * function will work for servers and clients. */
> + @Override public void send(FCPPluginMessage message) {
> + throw new NoSendDirectionSpecifiedException();
> + }
That this exists suggests that the send direction adapters actually
introduce a fair amount of complication. I suspect that removing them
would simplify things and avoid a lot of code.
...
Halfway through now - will resume at
985e34cc79b19b4412e8e3d3606cffeec5d4751b.
signature.asc
Description: OpenPGP digital signature
_______________________________________________ Devl mailing list Devl@freenetproject.org https://emu.freenetproject.org/cgi-bin/mailman/listinfo/devl
