-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256
This is part 2 of the review.
My mail client failed to save the draft of this, so I had to restore
from a text file copy of it which resulted in force-wrapping the code
too. If this is too hard to read let me know and I will redo it.
On 02/20/2015 03:04 AM, Arne Bab. wrote:
...
> # HG changeset patch # User xor-freenet <x...@freenetproject.org> #
> Date 1421696241 -3600 # Mon Jan 19 20:37:21 2015 +0100 # Node
> ID 985e34cc79b19b4412e8e3d3606cffeec5d4751b # Parent
> 9947f6d4f1790f9cc7ec447700f935c142fd7d92
> FCPPluginConnectionImpl.SendToClientAdapter: Optimize
...
> diff -r 9947f6d4f179 -r 985e34cc79b1
src/freenet/clients/fcp/FCPPluginConnectionImpl.java
...
> /** * Please use {@link
FCPPluginConnectionImpl#getDefaultSendDirectionAdapter(SendDirection)}
> * whenever possible to reuse adapters instead of creating
new ones with this constructor.*/
If that's the case does this constructor have to be visible to clients
at all?
> SendToClientAdapter(FCPPluginConnectionTracker tracker,
UUID connectionID) {
> super(SendDirection.ToClient); - this.tracker =
> tracker; - this.id = connectionID; + + //
> Reuse the WeakReference from the
FCPPluginConnectionTracker instead of creating our
> + // own one since it has to keep a WeakReference for
every connection anyway, and
> + // WeakReferences might be expensive to maintain for
the VM.
Unless we have done benchmarks indicating that WeakReferences have a
performance impact I'd like to shy from commenting as much. I'd be
fine if this comment were to go away.
> + try { + this.connectionRef =
tracker.getConnectionWeakReference(connectionID);
> + } catch (IOException e) { + // This
> function should only be used during
construction of the underlying
> + // FCPPluginConnectionImpl. While it is being
constructed, it should not be
> + // considered as disconnected already, and thus
> the
FCPPluginConnectionTracker
> + // should never throw IOException. +
> throw new RuntimeException("SHOULD NOT HAPPEN: " + e);
Does this have a constructor with a "cause" exception argument? Using
that might be preferable to concatenating it to a string.
Do you think it would be preferable to declare this constructor as
throwing an IOException?
> + } }
>
> @Override protected FCPPluginConnection getConnection()
throws IOException {
> - return tracker.getConnection(id); +
> FCPPluginConnection connection = connectionRef.get(); +
> if(connection == null) { + throw new
> IOException("Client has closed the
connection. "
> + + "Connection ID = " +
connectionRef.connectionID);
> + } + return connection; }
>
> @Override public UUID getID() { - return id; +
> return connectionRef.connectionID; } }
>
> # HG changeset patch # User xor-freenet <x...@freenetproject.org> #
> Date 1421697102 -3600 # Mon Jan 19 20:51:42 2015 +0100 # Node
> ID 7fcaf96a4528b46e999f4881f6e07e4cc78f1c0f # Parent
> 985e34cc79b19b4412e8e3d3606cffeec5d4751b
> FCPPluginConnectionTracker: Store FCPPluginConnection*Impl*
>
> instead of just FCPPluginConnection. This is necessary because new
> code in FCPServer soon to be committed will need functions of the
> Impl which do not make sense to add to the parent interface.
If the server needs them why don't they make sense in the interface?
This removes the flexibility to easily use another FCPPluginConnection
implementation, which does not make sense to me.
...
> # HG changeset patch # User xor-freenet <x...@freenetproject.org> #
> Date 1421697521 -3600 # Mon Jan 19 20:58:41 2015 +0100 # Node
> ID 26f3fc23479755d7b9d3da6b27d566fff5e6754d # Parent
> 7fcaf96a4528b46e999f4881f6e07e4cc78f1c0f FCPServer: Set default
> send direction in returned FCPPluginConnections
>
> .. by using the recently implemented
> getDefaultSendDirectionAdapter().
>
> diff -r 7fcaf96a4528 -r 26f3fc234797
src/freenet/clients/fcp/FCPServer.java
...
> @@ -524,6 +525,10 @@ * {@link
> PluginRespirator#connectToOtherPlugin(String, *
> FredPluginFCPMessageHandler.ClientSideFCPMessageHandler)}.
Plugins must use that instead.</p>
> * + * ATTENTION: Since this function is only to be used by the
aforementioned connectToPlugin()
> + * which in turn is only to be used by clients, the returned
connection will have a default send
> + * direction of {@link SendDirection#ToServer}.
If possible it would be nice if it were impossible to use the API in
incorrect ways instead of just having a warning in the documentation
against it.
> * @see FCPPluginConnectionImpl * The class JavaDoc of
> FCPPluginConnectionImpl explains
the code path for both networked
> * and non-networked FCP. @@ -537,18 +542,27 @@
> serverPluginName, messageHandler); // The constructor function
> already did this for us /*
> pluginConnectionTracker.registerConnection(connection); */
Please remove this commented-out code.
> - return connection; + return
connection.getDefaultSendDirectionAdapter(SendDirection.ToServer);
> }
>
> /** * <p><b>The documentation of {@link
FCPPluginConnectionTracker#getConnection(UUID)} applies to
> * this function.</b></p> * + * ATTENTION: Only for internal use
> by the frontend function + * {@link
> PluginRespirator#getPluginConnectionByID(UUID)}.
Plugins must use that instead.<br>
> + * <br>
Is there no way to restrict the visibility of this function so that
plugins cannot use it?
> + * ATTENTION: Since this function is only to be used by the
aforementioned
> + * getPluginConnectionByID() which in turn is only to be used
by servers, the returned
> + * connection will have a default send direction of {@link
SendDirection#ToClient}.
> + * * @see FCPPluginConnectionTracker * The JavaDoc of
> FCPPluginConnectionTracker explains the
general purpose of this mechanism.
> */ public final FCPPluginConnection getPluginConnectionByID(UUID
connectionID) throws IOException {
> - return
> pluginConnectionTracker.getConnection(connectionID); +
> return pluginConnectionTracker.getConnection(connectionID) +
.getDefaultSendDirectionAdapter(SendDirection.ToClient);
> }
>
> public PersistentRequestClient registerRebootClient(String name,
NodeClientCore core, FCPConnectionHandler handler) {
> # HG changeset patch # User xor-freenet <x...@freenetproject.org> #
> Date 1421697610 -3600 # Mon Jan 19 21:00:10 2015 +0100 # Node
> ID 6661a193d6dc6af1517a57f153b82c7b01de9817 # Parent
> 26f3fc23479755d7b9d3da6b27d566fff5e6754d
> FCPServer.createFCPPluginConnectionForIntraNodeFCP(): Trim return
> type
>
> Reducing this to the parent interface FCPPluginConnection will
> allow setting the visibility of FCPPluginConnectionImpl to package
> private since what this commit changes is IIRC the only place which
> uses FCPPluginConnectionImpl in a non-package-private way.
>
> - This was made possible by implementing the usage of
> getDefaultSendDirectionAdapter() in this function. I had speculated
> that this function would be called in a caller of the FCPServer
> function different package, which is why this used to return the
> *Impl. But now that we call it here already, the FCPServer doesn't
> need to return the Impl.
>
> diff -r 26f3fc234797 -r 6661a193d6dc
src/freenet/clients/fcp/FCPServer.java
> --- a/src/freenet/clients/fcp/FCPServer.java Mon Jan 19 20:58:41
2015 +0100
> +++ b/src/freenet/clients/fcp/FCPServer.java Mon Jan 19 21:00:10
2015 +0100
> @@ -533,7 +533,7 @@ * The class JavaDoc of
> FCPPluginConnectionImpl explains
the code path for both networked
> * and non-networked FCP. */ - public final
> FCPPluginConnectionImpl
createFCPPluginConnectionForIntraNodeFCP(
> + public final FCPPluginConnection
createFCPPluginConnectionForIntraNodeFCP(
How about naming this function something more like connectToPlugin()?
(Inter-node could be something like connectToRemotePlugin()) The
arguments suggest that this is not inter-node, and the documentation
can make it clear.
> String serverPluginName, ClientSideFCPMessageHandler
messageHandler)
> throws PluginNotFoundException {
>
> # HG changeset patch # User xor-freenet <x...@freenetproject.org> #
> Date 1421698282 -3600 # Mon Jan 19 21:11:22 2015 +0100 # Node
> ID 78bee089eb2fa9eaaa3a24512cc10b62f05a243c # Parent
> a314c83833894c988e0c9a546d6b5ffb6c21fc63 FCPPluginConnectionImpl:
> Trim visibility
>
> diff -r a314c8383389 -r 78bee089eb2f
src/freenet/clients/fcp/FCPPluginConnectionImpl.java
> --- a/src/freenet/clients/fcp/FCPPluginConnectionImpl.java Mon Jan
19 21:09:39 2015 +0100
> +++ b/src/freenet/clients/fcp/FCPPluginConnectionImpl.java Mon Jan
19 21:11:22 2015 +0100
> @@ -104,7 +104,7 @@ * <br/>This mechanism also works for
> networked FCP.<br> * </p></p> */ -public final class
> FCPPluginConnectionImpl implements
FCPPluginConnection {
> +final class FCPPluginConnectionImpl implements FCPPluginConnection
> {
>
> /** Automatically set to true by {@link Logger} if the log
level is set to
> * {@link LogLevel#DEBUG} for this class.
A general comment; not blocking: it is possible to convey this much
material in fewer words. It'd be nice to have conciseness up to the
point that it does not sacrifice clarity. For example this could
written as:
/** Set by {@link Logger} if the class log level includes {@link
* LogLevel#DEBUG}.
or one could reasonably argue that it's already evident from the name
what it does.
> # HG changeset patch # User xor-freenet <x...@freenetproject.org> #
> Date 1421701166 -3600 # Mon Jan 19 21:59:26 2015 +0100 # Node
> ID 91d2b74586bb4bd973f62e51e4082fd674c57d6a # Parent
> c426dc857d67ac35539bd19b93079fd6127c599a FCPPluginConnectionImpl:
> Fix "SHOULD NOT HAPPEN: java.io.IOException:"
>
> Unit test failure would be as follows:
>
> [junit] SHOULD NOT HAPPEN: java.io.IOException: FCPPluginConnection
> not found, maybe client has disconnected. Connection ID:
> 83665b37-32a6-41f3-9af7-5ca20db13324 [junit]
> java.lang.RuntimeException: SHOULD NOT HAPPEN: java.io.IOException:
> FCPPluginConnection not found, maybe client has disconnected.
> Connection ID: 83665b37-32a6-41f3-9af7-5ca20db13324 [junit] at
>
freenet.clients.fcp.FCPPluginConnectionImpl$SendToClientAdapter.<init>(FCPPluginConnectionImpl.java:1021)
> [junit] at
>
freenet.clients.fcp.FCPPluginConnectionImpl.<init>(FCPPluginConnectionImpl.java:376)
> [junit] at
>
freenet.clients.fcp.FCPPluginConnectionImpl.constructForUnitTest(FCPPluginConnectionImpl.java:435)
> [junit] at
>
freenet.clients.fcp.FCPPluginConnectionImplTest.testSendSynchronousThreadSafety(FCPPluginConnectionImplTest.java:38)
>
> diff -r c426dc857d67 -r 91d2b74586bb
src/freenet/clients/fcp/FCPPluginConnectionImpl.java
> --- a/src/freenet/clients/fcp/FCPPluginConnectionImpl.java Mon Jan
19 21:11:52 2015 +0100
> +++ b/src/freenet/clients/fcp/FCPPluginConnectionImpl.java Mon Jan
19 21:59:26 2015 +0100
> @@ -311,6 +311,9 @@ this.clientConnection = clientConnection;
> this.defaultSendDirectionAdapters.put(SendDirection.ToServer, new
> SendToServerAdapter(this)); + // new SendToClientAdapter()
> will need to query this
connection from the tracker already.
> + // Thus, we have to register before constructing it. +
> tracker.registerConnection(this);
> this.defaultSendDirectionAdapters.put(SendDirection.ToClient, new
> SendToClientAdapter(tracker, id)); } @@ -335,11 +338,8 @@
> assert(serverPluginName != null); assert(clientConnection !=
> null);
>
> - FCPPluginConnectionImpl result = new
FCPPluginConnectionImpl(tracker, executor,
> - serverPluginName,
serverPluginManager.getPluginFCPServer(serverPluginName),
> - clientConnection); -
> tracker.registerConnection(result); - return result; +
> return new FCPPluginConnectionImpl(tracker, executor,
serverPluginName,
> +
serverPluginManager.getPluginFCPServer(serverPluginName),
clientConnection);
> }
>
>
> @@ -373,6 +373,9 @@ this.clientConnection = null;
> this.defaultSendDirectionAdapters.put(SendDirection.ToServer, new
> SendToServerAdapter(this)); + // new SendToClientAdapter()
> will need to query this
connection from the tracker already.
> + // Thus, we have to register before constructing it.
This had to be put in two places. Getting rid of these adapter classes
could probably avoid such code duplication.
> + tracker.registerConnection(this);
> this.defaultSendDirectionAdapters.put(SendDirection.ToClient, new
> SendToClientAdapter(tracker, id)); }
...
> # HG changeset patch # User xor-freenet <x...@freenetproject.org> #
> Date 1421773177 -3600 # Tue Jan 20 17:59:37 2015 +0100 # Node
> ID 975557185f03b534046300d7688933ce8ce596c8 # Parent
> 3076f7d2292da011dbdbed5328cc9a65d5cad395
> ServerSideFCPMessageHandler: Allow keeping strong refs to
> connections
...
> - * Notice that there is no explicit disconnection
mechanism. Clients can come and go as
> - * they please. The only way to be sure that a
> connection
is alive is by checking whether
> - * the client replies to messages. You are encouraged
> to
make a "Ping" message with a
> - * "Pong" response a requirement for your server's
protocol.<br>
> + * if not. You may make a "Ping" message with a "Pong"
response a requirement for your
> + * server's protocol.<br>
Why isn't there an explicit disconnection mechanism? At the network
level there is one - closing the socket / sending a disconnect
message. Is the thought that no one would call a Java API equivalent?
It seems better to have this layer do transparent ping/pong instead of
suggesting that all the client applications reimplement it. At the
very least a default server-side implementation would be good. This
does not block merging.
...
> # HG changeset patch # User xor-freenet <x...@freenetproject.org> #
> Date 1421774244 -3600 # Tue Jan 20 18:17:24 2015 +0100 # Node
> ID 37ef59f5f65f86cbcfc3894ee3aa5f6d5142a40a # Parent
> 975557185f03b534046300d7688933ce8ce596c8 FCPPluginConnectionImpl:
> Detect bogus reply message identifiers
>
> The FredPluginFCPMessageHandler instructs users to always
> construct reply messages in a way mich makes them have the same
> identifier as the original messagge. Thus, we shouldn't allow reply
> messages which have a differing identifier.
>
> diff -r 975557185f03 -r 37ef59f5f65f
src/freenet/clients/fcp/FCPPluginConnectionImpl.java
> --- a/src/freenet/clients/fcp/FCPPluginConnectionImpl.java Tue Jan
20 17:59:37 2015 +0100
> +++ b/src/freenet/clients/fcp/FCPPluginConnectionImpl.java Tue Jan
20 18:17:24 2015 +0100
> @@ -787,6 +787,18 @@
>
> reply = null; } + +
> if(!reply.identifier.equals(message.identifier)) { +
> Logger.error(messageHandler,
"FredPluginFCPMessageHandler tried to send a"
> + + " reply with with different
identifier than original message."
> + + " See JavaDoc of its member
interfaces for how to do this properly."
Is there a way to make it the only possible way to send a reply? Hide
the identifier somehow?
> + + " connection = " +
FCPPluginConnectionImpl.this
> + + "; original message SendDirection =
> "
+ direction
> + + "; original message = " + message +
> + "; reply = " + reply); + + reply = null; +
> } } else if(reply == null) { if(!message.isReplyMessage()) { // The
> message handler did not not ship a
reply even though it would have
> # HG changeset patch # User xor-freenet <x...@freenetproject.org> #
> Date 1421774719 -3600 # Tue Jan 20 18:25:19 2015 +0100 # Node
> ID b4d2452d8d13fdad4eb93f3b3bcbfc5830dd6537 # Parent
> 37ef59f5f65f86cbcfc3894ee3aa5f6d5142a40a
> DefaultSendDirectionAdapter: Implement toString()
>
> As it is specified in FCPPluginConnection to contain debug
> information about the FCPPluginConnection, and
> FCPPluginConnectionImpl implements it to provide much information.
>
> diff -r 37ef59f5f65f -r b4d2452d8d13
src/freenet/clients/fcp/FCPPluginConnectionImpl.java
> --- a/src/freenet/clients/fcp/FCPPluginConnectionImpl.java Tue Jan
20 18:17:24 2015 +0100
> +++ b/src/freenet/clients/fcp/FCPPluginConnectionImpl.java Tue Jan
20 18:25:19 2015 +0100
> @@ -1111,6 +1111,15 @@ @Override public UUID getID() { return
> connectionRef.connectionID; } + + @Override public String
> toString() { + String prefix = "SendToClientAdapter for
> "; + try { + return prefix +
> getConnection(); + } catch(IOException e) { +
> return prefix + " FCPPluginConnectionImpl (" +
e.getMessage() + ")";
> + } + } }
>
> /** @@ -1147,6 +1156,10 @@ @Override public UUID getID() { return
> parent.getID(); } + + @Override public String toString() { +
> return "SendToServerAdapter for " + parent;
Is it intentional that there is no accompanying change in SendToClient?
> + } }
...
> # HG changeset patch # User xor-freenet <x...@freenetproject.org> #
> Date 1421775646 -3600 # Tue Jan 20 18:40:46 2015 +0100 # Node
> ID c44303d925deff911c63f0556ea89c33716c4cc3 # Parent
> 810ec77790f2aca7f900019ef380bae5367dff5d FCPPluginConnectionImpl:
> Detect non-reply replies
>
> The FredPluginFCPMessageHandler JavaDoc instructs users to always
> construct reply messages in a way which causes them to be marked
> as reply. Thus, we shouldn't allow reply messages which have
> !isReplyMessage().
>
> diff -r 810ec77790f2 -r c44303d925de
src/freenet/clients/fcp/FCPPluginConnectionImpl.java
> --- a/src/freenet/clients/fcp/FCPPluginConnectionImpl.java Tue Jan
20 18:38:51 2015 +0100
> +++ b/src/freenet/clients/fcp/FCPPluginConnectionImpl.java Tue Jan
20 18:40:46 2015 +0100
> @@ -786,6 +786,16 @@ + "; reply = " + reply);
>
> reply = null; + } else
> if(!reply.isReplyMessage()) { +
> Logger.error(messageHandler,
"FredPluginFCPMessageHandler tried to send a"
> + + " non-reply message as reply. See
JavaDoc of its member interfaces"
> + + " for how to do this properly."
Same comment as before - is there some way to make it so reply
messages must be marked as such?
> + + " connection = " +
FCPPluginConnectionImpl.this
> + + "; original message SendDirection =
> "
+ direction
> + + "; original message = " + message +
> + "; reply = " + reply); + + reply = null; }
> else
if(!reply.identifier.equals(message.identifier)) {
> Logger.error(messageHandler,
"FredPluginFCPMessageHandler tried to send a"
> + " reply with with different
identifier than original message."
In general it currently seems like the documentation is more verbose
than necessary, and that the API can rather easily be used
incorrectly. I'd like to look into making it harder to use it wrong;
the documentation being longwinded is less of a problem.
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2
iQIcBAEBCAAGBQJU80XBAAoJECLJP19KqmFu8jsP/jjJBZ00eUhVujj+/bR1qFKT
o6A+9IntSvV7u6gA5WFSHAWx41P57n85tK2yLo59e2ucN230YPJO8MWI9z8LOZ6A
IKu3gN2KPjCPMB7iRuvrARvgHqd49/QBQpDhxms/vToM+pA+vGEyj1ZLGNf4QWMj
ZIwXVXjuPt4GAaD6UcB3ih9wlus1+cddl1Pmw3HcvQLtwkY2NNjB481Tjj8RSyG2
5otHuRFTm/luaIfmYZ/QtrNzYUB0PVbpxWLhKQVujQffVDe55+QVhR6/6Lu59LvY
bhK/KrIixAJH1qc6sMC+8PkOk+QZDISs5nSHdOHl8z+kWl0yrMewJ6yI7l3dd23s
bJNePwJESTyUsAnRUP0kGwbhyFwW3DMfb/V51QEQngyoIExzTIfucc2HDMn764Oe
bMTwiS7Z19Uz5qGxegEQmVWunkg275RLG+CFplkx+jvYyP41ICdmJynrxRs0zfe7
jtMBvRDENhyRlo3EmaBfVPjXKP6is504qfpj/D2gGtxQ6gU/fduM3ujG7SetCKZJ
G3Fk8VXrbKjj3a1HNnYQOEUrqktIIxNXfGSLwcn/6g0NKw4wCi312OTxOCKXltfQ
3mrBZA9XQKbMpA/GzIoA48PrbZ34MU9EZtRd9ZF6x25W+sZ2A+oJ4P2i6SlzTLO/
uc+evnn74qu+eYmq/sWU
=PFGG
-----END PGP SIGNATURE-----
_______________________________________________
Devl mailing list
Devl@freenetproject.org
https://emu.freenetproject.org/cgi-bin/mailman/listinfo/devl
