On Sunday, March 01, 2015 12:01:04 PM Steve Dougherty wrote: > 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.
It's OK :) I have scrolled beyond any unquoted text which was not separated
by an empty line before / after. It seems like you always did add those in
your comments. Hope thats the case.
Readers please take notice of this though.
>
> 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?
Uhm, it is not visible to client users of the API: The constructor is package
private, and in fact its whole containing member class is private.
>
> > 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.
- I don't think without the comment it would be visible from the code why we
do this. Do you?
- The comment says that they "might" be expensive, i.e. doesn't wrongly claim
that I did a benchmark :)
Can we please refrain from having discussions about deleting 3 lines of
documentation? Too much documentation is a lot less severe than too little.
>
> > + 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.
Indeed it has! Nice catch - this is something which I did wrong frequently, I
was always only using the cause-version with 1 parameter (i.e. without
message), didn't know of the one which supports both a message and a cause.
You taught me something which I'll continue to use often :) Thanks.
Fixed:
https://github.com/xor-freenet/fred-staging/commit/499d78ec24a2c965a47bc6542d9df4d6c5f0e522
> Do you think it would be preferable to declare this constructor as
> throwing an IOException?
No, this is intentionally not done:
Declared exceptions are by convention part of normal operation, undeclared are
real errors. The big "SHOULD NOT HAPPEN" is there for a reason, this is not
planned to ever happen. Thus it wouldn't make sense to expect callers to catch
it.
> > + } }
> >
> > @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?
Short non-technical answer:
If the waiter (= server) at a restaurant needs access to functions of the
restaurant's internals (= the implementation FCPPluginConnectionImpl) to be
able to its job towards the customers (= API users), such as being able to
tell the cook "make 1000000 pizzas", does that mean the customers need access
to the same functions of the restaurant (= the interface FCPPluginConnection)
as well? No, the customer can use the server for communicating with the
restaurant's internal functions, and it probably wouldn't be a good idea if
the customer could directly send instructions to the restaurant's internal
cook anyway.
In other words: Public API interfaces shouldn't contain internal functions
which are needed to make the API work internally.
This example is even an understatement anyway: FCPServer is not a public API,
it is an internal class. So it is part of the implementation IMHO, and thus
free to use internal functions of the implementation without forcing them to
be in the interface.
Technical long answer:
The function which the server needs from the Impl is
getDefaultSendDirectionAdapter(). The SendDirectionAdapters are as you have
already noticed are not really simple to understand, and thus are currently
not even visible to the users of the API so we don't have to explain them to
them.
Also, they serve not only the purpose of providing a SendDirection, but also
to ensure that the server cannot keep a strong reference to the connection so
the client can force garbage collection of the connection by dropping all
strong references. This is necessary to make the disconnection mechanism of
the client dropping strong references actually work.
Now if we exposed getDefaultSendDirectionAdapter() to the interface, that
would mean that the server could get the adapter "send() defaults to sending
to the server" which the client normally only uses, and thus it would
implicitely get a strong reference to the connection, because thats what the
client-side adapter contains. As a result, I would have to reintroduce the
complex, cumbersome JavaDoc which tells servers to then not keep a strong
reference to the adapter at least. One of the primary points in introducing
the adapters was to eliminate that JavaDoc. With them, servers cannot keep a
strong reference to the connection because the adapter only keeps a weak one;
and they are only ever handed out directly the adapter.
Finally, the only thing which exposing getDefaultSendDirectionAdapter() by the
interface would allow is sending to yourself by default. That is an exotic
usecase anyway, not needed IMHO.
> This removes the flexibility to easily use another FCPPluginConnection
> implementation, which does not make sense to me.
The less functions an interface requires, the easier it is to implement it,
isn't it?
Also, if the stuff is not in the interface, but only in the implementation,
this means that only fred will be using it. So while it will be an annoyance
to have to fix FCPServer to be compatible against a new implementation, this is
still not significant because all those changes are still within fred, and
changing fred is easy compared to changing plugins.
Anyway, I hope we can agree that it would be very ugly to expose those complex
internal functions to plugins, and that implementation classes need the
ability to use internal functions of each others which aren't part of the
public interface.
>
> ...
>
> > # 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.
>
For clarity: The described problem here is that the function returns an
adapter whose send() by default will send to the *server*-side plugin of the
connection; so if you handed out the returned adapter to the server, the
server would be default always send() to itself, which is useless. It should
by default send to the client. So thats why the JavaDoc instructs the user to
notice the directionality of the returned adapter.
First of all, this is not "API". It is an internal function to be used by fred
code, not to be used by plugin authors. This is documented at the JavaDoc. (It
is public nevertheless for package structure reasons which are explained in
the comment second to next).
Internal functions are free to be more dangerous than API.
About fixing this to prevent wrong use: I don't see a way for making the
function determine *who* is calling it and what he does with the returned
object. I mean I cannot prevent people from adding fred code which hands out
the returned adapter to a server instead of to a client.
If you have any ideas how I could do this, please tell me.
> > * @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.
[You're suddenly jumping to class FCPPluginConnectionImpl, while the above and
below review results are from class FCPServer??]
Same reply as in the other review results:
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; + 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?
[notice that we seem to be back at class FCPServer here]
I also was disappointed by having to make this function public!
The reasons why I couldn't do otherwise are as follows:
- FCPServer is the right place for creating client connections obviously: The
server should deal with connection setup.
- However, we have the concept of class PluginRespirator being the primary
thing which plugins should use functions from. They shouldn't use fred
internals such as FCPServer directly. So PluginRespirator needs to contain
wrappers to the internal connection setup functions in FCPServer.
- FCPServer and PluginRespirator are in different packages, so I cannot make
the internal functions in FCPServer package private.
- Neither classes were created by this pull request, they have been like that
for a long time - existing plugins use PluginRespirator, and existing fred
code uses FCPServer - so I cannot move them around for being able to use
package private. Also, I think their current packages are properly chosen.
If you have any ideas how to resolve this, please tell me - I don't :(
>
> > + * 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.
This function is used in exactly 2 places and is also is not scheduled to be
used in further new code: It is only for internal fred use, not for use by
plugin authors.
For functions whose name will likely never have to be typed again, I prefer to
use self-explanatory names instead of short ones.
-> ACK to keep as is?
> > 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.
You're right, it has also annoyed me to have to add the lengthy comment about
this to every class. My primary motivation in doing this is that I think
people will not expect a random foreign class such as Logger to automatically
mess with another class's member variables. (I do think it is nice that its
capable of doing this though, and should stay as is.)
I just had another idea: Maybe we could create a custom annotation class such
as "@SetByClassLogger" which serves the purpose of containing this
documentation once and for all?
Do you want to quickfix this, file a bug for doing this, or shall I file a
bugtracker entry?
> > # 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>(FCPPl
> uginConnectionImpl.java:1021)
> > [junit] at
>
> freenet.clients.fcp.FCPPluginConnectionImpl.<init>(FCPPluginConnectionImpl.j
> ava:376)
> > [junit] at
>
> freenet.clients.fcp.FCPPluginConnectionImpl.constructForUnitTest(FCPPluginCo
> nnectionImpl.java:435)
> > [junit] at
>
> freenet.clients.fcp.FCPPluginConnectionImplTest.testSendSynchronousThreadSaf
> ety(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.
Hmm. Nice catch. A quick workaround could be to move the registration to the
constructor which needs it to have happened already. But this would be ugly,
as the constructor of an object of type X shouldn't register an object of type
Y, X should deal with that itself.
With regards to removing the adapter classes, what I said in the previous
review results applies: Removing them now would be too much work.
There's one good thing here though: I think I don't have to document your idea
for the case when someone removes the adapters. Both the previous and the
second-to-next lines after the comments we're talking about call functions of
the adapters. So once someone removes the adapters, he will very likely read
the comment as well and remove it.
-> ACK to keep as is?
>
> > + 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?
There are two aspects here depending who you are:
1) From the perspective of the client / the one who has to call a hypothetical
close():
Instead having garbage collection be the implicit disconnection mechanism as
it is now prevents people from forgetting to call close().
2) The server / the one who would NOT call close():
If the other side just pulls the network cable out of the wall, you will never
get the signal that the other side close()d. So the "you were close()d" signal
would be unreliable, and thus of no benefit. (Also, forced disconnection is a
regular thing for many people: Most German ISPs disconnect your Internet
connection every 24 hours. Now of course FCP isn't intended to be used across
the Internet yet. But I think something which aims to be a network protocol
should use the basic assumptions behind any Internet protocol. We can never
know what people will do with it.)
It could be made reliable with a "ping" mechanism, however I decided against
implementing it for the following reason...
> 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.
I have also considered making automatic ping/pong part of the code. However I
decided against it for robustness purposes: If ping/pong happens at fred
layer, and not at client plugin layer, this also means that the client plugin
layer is not tested by ping/pong. I.e. by having ping/pong flow through all
layers, you know that all layers are working.
But for WOT at least, there is class FCPClientReferenceImplementation which
provides automated ping/pong (and even reconnect). So at least people who want
to use WOT can have an existing implementation.
If we wanted to add an explicit close() in the future, we could easily do this
in a backwards compatible way:
- Adding new stuff (= close()) to the FCPPluginConnection interface will only
require fred-side implementations to be changed to implement it. Disturbing
fred-side implementations is OK compared to disturbing plugins.
- The existing code which uses garbage collection monitoring to notice closing
of the connection could be recycled to call the new close() while existing
client applications have not been adapted to call it yet. I.e. we could call
close() for them while they don't bother to do so yet.
- If we want to provide the ability for servers to receive a "client
connection was closed" callback, we could put that callback into a separate
interface which can optionally be implemented by servers. Since the new
interface is optional, this is also backwards compatible.
Anyway, you said its not a blocker already, so I consider this as resolved for
now :)
> ...
>
> > # 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?
I see the following possibilities of enforcing the returned reply to be the
contain the same identifier, both of which aren't nice IMHO:
1) Ignore the identifier and always assume the correct identifier. This would
only hide the problem, it would not fix the fact that the handler returns weird
stuff. Remember: The only two ways you can get the identifier wrong in what you
return in the handler is by using the wrong constructor of FCPPluginMessage,
i.e. not constructREPLY...() but construct(), or by actually replying to a
different message. The client is actually acting seriously wrong then, so I
would rather not do this fix of hiding the problem, its better if we do catch
the problem.
2) Pass a pre-constructed reply to the message handler. This would make the
code...
- Slower: It would construct a reply even if the handler doesn't want to reply
then.
- Less robust: FCPPluginMessage right now has all its member variables be
final. They would have to be changed to be non-final so the handler can modify
them, since it cannot specify them at construction anymore.
- Ugly: A message handler should receive the message it has to handle, not the
reply it will send. Message goes in, reply goes out. Reply doesn't go in.
-> ACK to keep as is?
> > + + " 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?
Uhm, you yourself right above just quoted code which adds a toString() to
SendToClientAdapter?
> > + } }
>
> ...
>
> > # 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?
Reply messages *are* marked as reply by having FCPPluginMessage.success !=
null.
But your primary complaint seems to be that it is not enforced by the design
of the message handler interface that it can only return reply messages.
We right now cannot enforce the message handler to return a reply message
because it can construct the FCPPluginMessage object on its own, and thus use
the wrong construct...() function.
I suppose a way to make it possible to enforce it to return a reply message
would be to split FCPPluginMessage into two classes so that "class
FCPPluginReplyMessage extends FCPPluginMessage" and to change the message
handler interface specification to have a return type of FCPPluginReplyMessage.
Reasons why I did it the way it is, without a second class for replies, and
also reasons for keeping it as is for now:
- There are already more classes than enough to represent messages. For
example there is also class FCPPluginServerMessage and class
FCPPluginClientMessage which are the internal representations, while
FCPPluginMessage is the external one for use by plugins.
Then there are the nodes core FCP message classes, which are probably over a
dozen...
- Class FCPPluginMessage is large enough already, adding a second class will
make understanding the whole stuff even more difficult. I like the concept of
having a single class which people can read to understand a single thing.
- Also, it is more symmetrical to have only one class for messages.
FCPPluginMessage goes in, FCPPluginMessage goes out.
- It would be a whole week of work possibly, there is lots of JavaDoc which
would have to be updated.
- It is not that complex for users to use it the right way, i.e. return a
reply message when you are supposed to, as it is now. FCPPluginMessage has no
publicly visible constructors, it only has "construct..." functions -
"construct()", "constructSuccessReply()", "constructErrorReply()". So you
already see from the name of the function you are using to construct it
whether it is a reply.
I do notice that this is one of the changes where you might be tempted to
force me to do it now because in the future changing the return type of the
message handling function to a new class which is a child class of the old is
not backwards compatible. However, I am still for postponing this since I
actually had an idea how to implement this in the future in a somewhat
compatible way by:.
1) Adding a class FCPPluginReplyMessage extends FCPPluginMessage as suggested
2) Keeping the interface for the message handler as is, i.e. return
FCPPluginMessage. Notably, plugin authors will be free to now return the new
FCPPluginReplyMessage nevertheless without the compiler complaining because it
does extend FCPPluginMessage.
3) Deprecate the message handling functions and in the deprecation JavaDoc say
"This function is not actually deprecated, you may continue using it The
deprecation is to notice you to please change the return type of your
implementations to FCPPluginReplyMessage soon, as we want to change the return
type in the interface.".
4) Wait some time until all plugin authors have converted the return type.
5) Change the return type. While this formally modifies the interface, plugins
should still have had a change to be fixed to compile against the new return
type due to the deprecation message; while after having been changed their
return type still be compile-compatible against the old return type meanwhile
because the new class extends the old.
If thats too hack-ish for you, another more complex but more clean migration
path in the future might be to:
1) Add a new message handler interface with a FCPPluginReplyMessage as return
type.
2) Deprecate the old message handler interface
3) For plugins which do not implement the new interface yet, call their old
interface handler and convert the returned FCPPluginMessage in a sane way to
FCPPluginReplyMessage.
So anyway, given that this would be a week of work to do now, and that there
are somewhat backwards-compatible ways to change this in the future, ACK to
keep as is?
> > + + " 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,
Such generic "all of it is [...]" criticism is very difficult to reply against.
Honestly, I would rather have hoped to get a compliment to have bothered to
write something which is fully documented :) It's not like fred is a forest of
documentation.
And please remember the reason why I started to rewrite the whole plugin FCP
stuff: I figured that I could not repair the old API instead because it was not
documented and thus plugin authors used it in different, incompatible
interpretations of how it should be used. If there was a documentation which
specified how it should be used, the whole rewrite would not have been
necessary probably.
Last but not least, to reply with another generic thing: The lectures I had
about software quality / software engineering had the general vibe of
"documentation, documentation, documentation". The people who said that have
more experience than me, so if they say that lack of documentation crashed a
lot of software projects, I trust them.
And I would say that it especially applies when writing an API since many
different programmers will use APIs.
> 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.
Same as above: Hard to reply to a general "everything of it can be used wrong"
complaint.
From briefly reading over the review results again, I think you often confused
internal functions with public parts of the API, which might explain what you
just said here. Internal functions are supposed to be less restrictive.
With regards to the non-internal ones which can be abused: And as far as I
know, the documentation at least fully specifies how they should be used
correctly, doesn't it? I tried to make everything very well-defined, I hope
there shouldn't be any places where a documentation how to use it correctly is
lacking.
So at least we can point the finger at the users if they're using it wrong,
because they were told how to use it right :D
Additionally, I think most cases of wrong usage by the public parts should at
least trigger ERROR logging :)
Remember the problem with the previous API was that it wasn't even documented
how it is used correctly; so at least now we have documentation, still better
than nothing :)
Anyway, I enjoyed reading your review results and hope the fixes I provided are
enough :) Thanks a lot for bothering with the whole of this. I also hope the
general climate of my replies has improved over the previous IRC discussions
:)
Please make sure to ACK/NACK everything so I know how to proceed.
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
