On 03/07/2015 09:31 PM, xor wrote:
> On Saturday, February 21, 2015 06:17:45 PM Steve Dougherty wrote:
>>> # 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?)
>
> As you've noticed, we cannot do this since messages do not contain a
> "Synchronous=True/False" flag.
> You are right about the possible optimization of making messages contain
> a "Synchronous=True" flag so we can avoid checking the table every time.
>
> I didn't add this yet because people strongly demanded the code to be network
> transparent at the previous draft pull request. The transparency is that is
> that the "synchronous" attribute of messages does not exist at the network
> protocol level. All messages are asynchronous, and the synchronous API is
> merely a bonus feature of the implementation. This allows the remote
> implementations to be completely asynchronous if they want to be simple.
> There arguably is some performance reason beyond code quality for this:
> Network traffic is usually assumed to be much slower and more expensive than
> CPU
> time, so if something can be implemented by using more CPU instead of having
> an additional field in the network messages, it shall be done this way.
>
> [The same reasoning applies to not having a SendDirection=ToServer/ToClient
> flag: We can determine this by CPU computations, without adding overhead to
> the
> network format.]
>
> Also notice that there IS some level of optimization by content of the
> messages already already:
> if(!message.isReplyMessage()) {
> return false; /* Editor's note: Not synchronous */
> }
>
> Messages contain a flag "Success=true/false" which implicitly indicates
> whether they are a reply - a reply always contains a Success flag, a
> non-reply
> always does not.
> So half of the messages will not result in a check of the
> sendSynchronous thread-table.
>
> With regards to backwards compatibility, I would say we can postpone this
> optimization until profiling some days shows that it is a bottleneck:
> - It is possible to add new fields to FCP messages in the future without
> breaking existing clients, they will just ignore the new field.
> - Once we add the new flag, the code will still be backwards compatible to
> old
> clients: If the client doesn't provide a "Synchronous=True/False" flag, we
> can
> just implicitly assume it to be True then, and check the send table for
> whether a waiter is existing. If not, we can safely continue to ship the
> message as if it was non-synchronous.
>
> Thus, I've added a TODO about this at commit:
> https://github.com/xor-freenet/fred-staging/commit/b918ecdf07b621a7b6d5c6f1416740993c3def00
>
> ACK if you're OK with postponing this.
Network transparency remains a requirement yes. I wasn't suggesting
optimization here, but rather changes for readability. (and shorter names!)
Would it be reasonable to add possibly-thread-table-derived knowledge of
whether it is synchronous to the message class itself to make this code
easier to read?
>>> # 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?)
>
> Yes, the sendSynchronous() provides the guarantee of waiting for delivery of
> the message, which is why send() returns immediately.
> Actually sendSynchronous() even waits for the reply to arrive.
Okay, good. A note on what to use to get those additional guarantees
would be useful here.
> I would assume what you're demanding is some kind of mixture between send()
> and sendSynchronous() - wait for the message to have been deployed to the
> network but do not wait for a reply?
I'm not demanding anything here, and no, that's not what I'm asking for.
I was unclear on whether there was something with more guarantees, and
whether sendSynchronous() was that thing.
> My experience with various network APIs shows that such a thing typically
> doesn't exist by convention - either something is fully asynchronous, or
> fully
> synchronous. This is probably because a hybrid has no real benefit: Just
> because you put something into the send-buffer of your network card doesn't
> give you any guarantees whatsoever. The receive-buffer of the remote side
> might
> still overflow if you don't wait for replies before sending the next message.
> You also have no guarantee of the messages arriving, the remote side might
> have gone offline after you put the stuff in the network buffer. (Besides, it
> might not even be possible to determine when something was put into the
> buffer
> by Java API restrictions.)
> Thus, as I cannot think of any use of a hybrid, and as this specification of
> send() follows conventions of network APIs in general, I would suggest we
> leave this as is.
>
> ACK?
I agree that would be a mix of strange, confusing, and pointless.
>>> + */
>>> + @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.
>
> - Package private code will not be accessible to users of the API, plugins
> will not be in this package. They are by convention in packages such as
> "plugins.WebOfTrust.*". Plugin authors won't even see class
> FCPPluginConnectionImpl, the whole class is package private. They will only
> see the FCPPluginConection interface, with doesn't contain this function. In
> other words: I did not add this comment for plugin authors, I merely added it
> for internal fred developers. So this doesn't affect much stuff as you might
> have thought.
Oh cool, alright.
> - FCPConnectionHandler is a class for handling networked FCP messages, it
> doesn't exist for non-networked plugin-to-plugin connections. Also, it can
> exist for connections which do not even interact with plugins. Thus, I don't
> currently see a way of merging this with class FCPPluginConnectionImpl to
> have
> this function private, as FCPConnectionHandler is a special case, it doesn't
> exist for all FCPPluginConnectionImpl, and vice versa. So I currently cannot
> think of a way to resolve this by class structure as you suggested. If you
> can
> provide an idea, I will add it as TODO :)
> - I agree to have this postponed as the whole stuff isn't visible to users of
> the API as explained above.
Sure.
>> ...
>>
>>> # 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.)
>
> The serverPluginName documentation has been fixed at commit
> https://github.com/xor-freenet/fred-staging/commit/d5f2f3bb54d40f75c9e06ad698ee117265b77e8b
>
>> ...
>>
>>> # 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?
>
> Fixed by
> https://github.com/xor-freenet/fred-staging/commit/541de36566fb80adb85caceb676e3b2608dbc707
>
>> ...
>>
>>> # 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.
>
> This is to follow the convention of the network and file I/O API of Java:
> http://docs.oracle.com/javase/7/docs/api/java/net/Socket.html
>
>> Is the thought that the reason
>> behind the failure doesn't change the possible recovery?
>
> Thats also a good observation :)
> - Network is something which often fails, thus failures are sort of normal
> and
> we don't care much about why a message couldn't be delivered. It's only
> important to know that we have to retry sending it, which IOException
> indicates.
> The only distinction is that the message might have actually been delivered,
> but processing might have failed at the remote side. This will be indicated
> by
> a different way: sendSynchronous() will NOT throw IOException, but DO return
> a
> reply. The reply will have FCPPluginMessage.success == false. This is also
> explained in the JavaDoc :)
>
> If someone in the future *does* need to know if a timeout happened, we could
> implement that in a backwards-compatible way then: Add a "class
> TimeoutException extends IOException".
>
> -> ACK to keep as is?
Sure, that sounds solid.
>>> + + " 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.
>
> There is only a single FCPPluginConnectionImpl object for each connection
> between a server and a client, thus we cannot store a default SendDirection
> as
> a member variable. The reasons why I decided against having two different
> FCPPluginConnectionImpl objects for server and client are:
> - There *is* only one connection between two connection partners. Example:
> This is same as there is only a single cable between us if I call you by
> phone.
> - It allows the stuff to eliminate code duplication. This is visible in the
> implementation of send(final SendDirection direction, FCPPluginMessage
> message).
> - The previous API which used different connection objects depending on
> whether
> one is server or client seemed sort of difficult to understand to me.
>
> Also, notice that there were no default SendDirection adapters when I
> initially wrote the code. People then rightfully complained in the review
> that
> having to always specify the SendDirection in send() is annoying. Adding the
> adapters was then suggested by bertm as an easy way to refactor the code. And
> it indeed was very little work, and solved two problems at once. (The other
> problem which this solves was that previously the JavaDoc had to forbid
> servers to keep strong references to the FCPPluginConnection so the client
> can
> disconnect by dropping all strong references and thus causing GC. This is
> solved by the adapters by having a WeakReference in the adapter which we hand
> to the server).
>
> Overall, I agree that this is a somewhat complex thing if you look at it
> without knowing how it came to exist. However, I would say that changing this
> would result in a rewrite of the whole FCPPluginConnection implementation -
> we
> would probably have to split it into multiple classes, and change the whole
> stuff to DO create two FCPPluginConnectionImpl objects for server/client.
> Thus, I am strongly biased to wanting to keep this as is.
> It for sure can be changed in the future in a backwards compatible way as the
> adapters are not externally visible from the API, they are an implementation
> detail.
> -> ACK to keep as is?
It may eliminate code duplication in some cases, but it adds it in
others (as well as that class without a direction that throws an error
for everything? why does that exist?). It would be nice to avoid the
various ways in which these classes can be uninitialized or in illegal
states. Especially given that the distinction is not externally visible
(yay!) I'm fine with this as-is for now. It'd be nice for commit
messages (or maybe even in-code documentation) to include this design
reasoning. (Or does it and I missed it?)
>>
>> ...
>>
>>> # 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.
>
> See above.
>
>>
>> ...
>>
>>> # 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.
>
> Users of the API cannot specify these directly as they only can construct
> FCPPluginConnectionImpl through other functions, they cannot access these
> constructors. Thus, those references could only maliciously set to null by
> node-side code. The node-side code is assumed to be a safe environment, and
> thus I think asserts are sufficient.
>
> There was one case where the client *could* specify one of these, for which
> I've added a null-check at commit:
> https://github.com/xor-freenet/fred-staging/commit/8a02a024df7dd99cb32245cc858e6dcb445a19cb
>
> -> ACK to keep as is?
Sure.
>> ...
>>
>>> * 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.
>
> Same reply as above :)
>
>> ...
>>
>>> 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.
>
> This code was not left commented out there as a temporary measure of removing
> obsolete code, but specifically to tell a careful reader that it is not
> necessary here to do this / to prevent them from adding it.
> In other words: This is documentation.
> -> ACK to keep as is?
I go into more detail in the second half of the review, but it was not
clear to me that this was documentation.
>>> 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?
>
> For SendToClientAdapter: Time-performance: Constructing such an object
> requires calling FCPPluginConnectionTracker.getConnectionWeakReference(),
> which needs a read-lock of a ReadWriteLock, and a query in a TreeMap (=
> something around O(N LOG(N)).
>
> For SendToServerAdapter: Time-performance, albeit less severe: We don't want
> to have to construct a fresh SendToServerAdapter object for every send().
> Constructing objects is said to be expensive in Java.
>
> I've documented the above at commits
> https://github.com/xor-freenet/fred-staging/commit/db70b5248739f82b9cf4e7359745d08a650d0a7b
>
> https://github.com/xor-freenet/fred-staging/commit/f2e61a4378c1121db8089dbbbc4d3059f62e7d60
>
>> Is there the danger that it will be stale?
>
> It contains a WeakReference, which can be null. But that is planned, and
> handled.
>
>> Is the
>> complication of making sure it's reinitialized worth it? (Did
>> benchmarking indicate it was impacting performance?)
>
> There is no complex re-initialization if you re-use the existing object. Its
>
> functions will always check the WeakReference of whether its null or not.
I'm curious whether this optimization was suggested by profiling?
> -> ACK to keep as is with the new JavaDoc?
Okay.
>> ...
>>
>>> # 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.
>
> Users of the API will never run into this exception. They will only be given
> an adapter, they will never receive a FCPPluginConnectionImpl itself. So they
> cannot call this always-throwing function.
If API users cannot call it why does the class need to exist?
> Besides that, my lengthy explanation above in the mail applies again:
> Removing
> the adapters would be a lot of work, and can be done later on.
>
> -> ACK?
>
signature.asc
Description: OpenPGP digital signature
_______________________________________________ Devl mailing list Devl@freenetproject.org https://emu.freenetproject.org/cgi-bin/mailman/listinfo/devl
