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.
> > # 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.
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?
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?
> > + */
> > + @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.
- 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.
> ...
>
> > # 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?
> > + + " 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?
>
> ...
>
> > # 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?
> ...
>
> > * 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?
> > 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.
-> ACK to keep as is with the new JavaDoc?
> ...
>
> > # 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.
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: This is a digitally signed message part.
_______________________________________________ Devl mailing list Devl@freenetproject.org https://emu.freenetproject.org/cgi-bin/mailman/listinfo/devl
