PROTON-881: tidy up comments and TODO's Remove TODO's if they were already done, downgrade some TODO's to comments (if they were simply observations), and remove some comments that consisted of proton-c code - pasted in as a reference.
Project: http://git-wip-us.apache.org/repos/asf/qpid-proton/repo Commit: http://git-wip-us.apache.org/repos/asf/qpid-proton/commit/46b9d848 Tree: http://git-wip-us.apache.org/repos/asf/qpid-proton/tree/46b9d848 Diff: http://git-wip-us.apache.org/repos/asf/qpid-proton/diff/46b9d848 Branch: refs/heads/master Commit: 46b9d848999e993f238b37c9a0598035ccd64b27 Parents: d6c4ba7 Author: Adrian Preston <prest...@uk.ibm.com> Authored: Thu May 7 01:21:41 2015 +0100 Committer: Adrian Preston <prest...@uk.ibm.com> Committed: Thu May 7 01:21:41 2015 +0100 ---------------------------------------------------------------------- .../org/apache/qpid/proton/reactor/Reactor.java | 13 ++-- .../apache/qpid/proton/reactor/Selectable.java | 3 +- .../qpid/proton/reactor/impl/AcceptorImpl.java | 2 +- .../qpid/proton/reactor/impl/IOHandler.java | 14 ++-- .../qpid/proton/reactor/impl/ReactorImpl.java | 81 ++------------------ .../reactor/impl/ReactorInternalException.java | 21 +++++ .../qpid/proton/reactor/impl/SelectorImpl.java | 4 +- 7 files changed, 43 insertions(+), 95 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/qpid-proton/blob/46b9d848/proton-j/src/main/java/org/apache/qpid/proton/reactor/Reactor.java ---------------------------------------------------------------------- diff --git a/proton-j/src/main/java/org/apache/qpid/proton/reactor/Reactor.java b/proton-j/src/main/java/org/apache/qpid/proton/reactor/Reactor.java index e658057..f91f376 100644 --- a/proton-j/src/main/java/org/apache/qpid/proton/reactor/Reactor.java +++ b/proton-j/src/main/java/org/apache/qpid/proton/reactor/Reactor.java @@ -56,14 +56,11 @@ public interface Reactor { public void setHandler(Handler handler); -/* TODO - * pn_io_t *pn_reactor_io(pn_reactor_t *reactor) { -166 assert(reactor); -167 return reactor->io; -168 } -169 - - */ + // XXX: The C reactor has a pn_reactor_io() function. The closest Java equivalent + // would be a factory for creating SocketChannel's, ServerSocketChannelsm and Selectors. + // This seems like overkill unless there's a use for this in unit testing, or the + // Reactor needs to be integrated with an exotic Java environment which provides its + // own networking implementation. public Set<ReactorChild> children(); http://git-wip-us.apache.org/repos/asf/qpid-proton/blob/46b9d848/proton-j/src/main/java/org/apache/qpid/proton/reactor/Selectable.java ---------------------------------------------------------------------- diff --git a/proton-j/src/main/java/org/apache/qpid/proton/reactor/Selectable.java b/proton-j/src/main/java/org/apache/qpid/proton/reactor/Selectable.java index 1bdaa1e..e839b10 100644 --- a/proton-j/src/main/java/org/apache/qpid/proton/reactor/Selectable.java +++ b/proton-j/src/main/java/org/apache/qpid/proton/reactor/Selectable.java @@ -67,9 +67,10 @@ public interface Selectable extends ReactorChild { void release() ; + @Override void free() ; - // These are equivalent to the C code's set/get file descritor functions. + // These are equivalent to the C code's set/get file descriptor functions. void setChannel(SelectableChannel channel) ; public SelectableChannel getChannel() ; http://git-wip-us.apache.org/repos/asf/qpid-proton/blob/46b9d848/proton-j/src/main/java/org/apache/qpid/proton/reactor/impl/AcceptorImpl.java ---------------------------------------------------------------------- diff --git a/proton-j/src/main/java/org/apache/qpid/proton/reactor/impl/AcceptorImpl.java b/proton-j/src/main/java/org/apache/qpid/proton/reactor/impl/AcceptorImpl.java index 38d5416..243a963 100644 --- a/proton-j/src/main/java/org/apache/qpid/proton/reactor/impl/AcceptorImpl.java +++ b/proton-j/src/main/java/org/apache/qpid/proton/reactor/impl/AcceptorImpl.java @@ -97,7 +97,7 @@ public class AcceptorImpl implements Acceptor { sel = ((ReactorImpl)reactor).selectable(this); sel.setChannel(ssc); sel.onReadable(new AcceptorReadable()); - sel.onFree(new AcceptorFree()); // TODO: currently, this is not called from anywhere!! + sel.onFree(new AcceptorFree()); sel.setReactor(reactor); sel.setAttachment(handler); sel.setReading(true); http://git-wip-us.apache.org/repos/asf/qpid-proton/blob/46b9d848/proton-j/src/main/java/org/apache/qpid/proton/reactor/impl/IOHandler.java ---------------------------------------------------------------------- diff --git a/proton-j/src/main/java/org/apache/qpid/proton/reactor/impl/IOHandler.java b/proton-j/src/main/java/org/apache/qpid/proton/reactor/impl/IOHandler.java index 6199c56..872b6ff 100644 --- a/proton-j/src/main/java/org/apache/qpid/proton/reactor/impl/IOHandler.java +++ b/proton-j/src/main/java/org/apache/qpid/proton/reactor/impl/IOHandler.java @@ -99,7 +99,7 @@ public class IOHandler extends BaseHandler { } Transport transport = event.getConnection().getTransport(); - Socket socket = null; // TODO: null is our equivalent of PN_INVALID_SOCKET + Socket socket = null; // In this case, 'null' is the proton-j equivalent of PN_INVALID_SOCKET try { SocketChannel socketChannel = SocketChannel.open(); socketChannel.connect(new InetSocketAddress(hostname, port)); @@ -111,7 +111,7 @@ public class IOHandler extends BaseHandler { transport.setCondition(condition); transport.close_tail(); transport.close_head(); - transport.pop(transport.pending()); // TODO: force generation of TRANSPORT_HEAD_CLOSE (not in C code) + transport.pop(transport.pending()); // Force generation of TRANSPORT_HEAD_CLOSE (not in C code) } selectableTransport(reactor, socket, transport); } @@ -183,9 +183,9 @@ public class IOHandler extends BaseHandler { transport.close_tail(); } } - // TODO: comment from C code... - // occasionally transport events aren't generated when expected, so - // the following hack ensures we always update the selector + // (Comment from C code:) occasionally transport events aren't + // generated when expected, so the following hack ensures we + // always update the selector update(selectable); reactor.update(selectable); } @@ -264,8 +264,8 @@ public class IOHandler extends BaseHandler { } // pn_reactor_selectable_transport + // Note the socket argument can, validly be 'null' this is the equivalent of proton-c's PN_INVALID_SOCKET protected static Selectable selectableTransport(Reactor reactor, Socket socket, Transport transport) { - // TODO: this code needs to be able to deal with a null socket (this is our equivalent of PN_INVALID_SOCKET) Selectable selectable = reactor.selectable(); selectable.setChannel(socket != null ? socket.getChannel() : null); selectable.onReadable(new ConnectionReadable()); // TODO: *IF* these callbacks are stateless, do we more than one instance of them? @@ -296,7 +296,7 @@ public class IOHandler extends BaseHandler { ReactorImpl reactor = (ReactorImpl)event.getReactor(); Selector selector = reactor.getSelector(); if (selector == null) { - selector = new SelectorImpl(); // TODO: the C code supplies the reactor's pn_io object here... + selector = new SelectorImpl(); reactor.setSelector(selector); } http://git-wip-us.apache.org/repos/asf/qpid-proton/blob/46b9d848/proton-j/src/main/java/org/apache/qpid/proton/reactor/impl/ReactorImpl.java ---------------------------------------------------------------------- diff --git a/proton-j/src/main/java/org/apache/qpid/proton/reactor/impl/ReactorImpl.java b/proton-j/src/main/java/org/apache/qpid/proton/reactor/impl/ReactorImpl.java index cde5f42..c2c30ef 100644 --- a/proton-j/src/main/java/org/apache/qpid/proton/reactor/impl/ReactorImpl.java +++ b/proton-j/src/main/java/org/apache/qpid/proton/reactor/impl/ReactorImpl.java @@ -47,23 +47,6 @@ import org.apache.qpid.proton.reactor.Task; public class ReactorImpl implements Reactor { - /* - * pn_record_t *attachments; - 41 pn_io_t *io; - 42 pn_collector_t *collector; - 43 pn_handler_t *global; - 44 pn_handler_t *handler; - 45 pn_list_t *children; - 46 pn_timer_t *timer; - 47 pn_socket_t wakeup[2]; - 48 pn_selectable_t *selectable; - 49 pn_event_type_t previous; - 50 pn_timestamp_t now; - 51 int selectables; - 52 int timeout; - 53 bool yield; - */ - private Object attachment; private CollectorImpl collector; private long now; @@ -77,6 +60,7 @@ public class ReactorImpl implements Reactor { private Type previous; private Timer timer; private final Pipe wakeup; + private Selector selector; @Override public long mark() { @@ -88,25 +72,7 @@ public class ReactorImpl implements Reactor { public long now() { return now; } -/* - * tatic void pn_reactor_initialize(pn_reactor_t *reactor) { - 68 reactor->attachments = pn_record(); - 69 reactor->io = pn_io(); TODO: pn_io most literally translates to SocketFactory (and possibly also ServerSocketFactory...) - 70 reactor->collector = pn_collector(); - 71 reactor->global = pn_iohandler(); - 72 reactor->handler = pn_handler(NULL); - 73 reactor->children = pn_list(PN_OBJECT, 0); - 74 reactor->timer = pn_timer(reactor->collector); - 75 reactor->wakeup[0] = PN_INVALID_SOCKET; - 76 reactor->wakeup[1] = PN_INVALID_SOCKET; - 77 reactor->selectable = NULL; - 78 reactor->previous = PN_EVENT_NONE; - 79 reactor->selectables = 0; - 80 reactor->timeout = 0; - 81 reactor->yield = false; - 82 pn_reactor_mark(reactor); - 83 } - 84 */ + public ReactorImpl() throws IOException { collector = (CollectorImpl)Proton.collector(); global = new IOHandler(); @@ -184,15 +150,6 @@ public class ReactorImpl implements Reactor { this.handler = handler; } -/* TODO - * pn_io_t *pn_reactor_io(pn_reactor_t *reactor) { -166 assert(reactor); -167 return reactor->io; -168 } -169 - - */ - @Override public Set<ReactorChild> children() { return children; @@ -248,13 +205,7 @@ public class ReactorImpl implements Reactor { } } - // TODO: pn_record_get_handler - // TODO: pn_record_set_handler - // TODO: pn_class_reactor - // TODO: pn_object_reactor - // TODO: pn_event_reactor - - // pn_event_handler - TODO: this is copied from the Reactor.java code, so might need some tweaks... + // pn_event_handler private Handler eventHandler(Event event) { Handler result; if (event.getLink() != null) { @@ -269,10 +220,6 @@ public class ReactorImpl implements Reactor { result = ((HandlerEndpointImpl)event.getConnection()).getHandler(); if (result != null) return result; } -// if (event.getTransport() != null) { // TODO: do we want to allow handlers to be added to the Transport object? -// result = ((EndpointImpl)event.getTransport()).getHandlers(); -// if (result.hasNext()) return result; -// } if (event.getTask() != null) { result = event.getTask().getHandler(); @@ -347,7 +294,6 @@ public class ReactorImpl implements Reactor { @Override public void wakeup() throws IOException { - //selector.wakeup(); wakeup.sink().write(ByteBuffer.allocate(1)); // TODO: c version returns a value! } @@ -355,7 +301,6 @@ public class ReactorImpl implements Reactor { public void start() { collector.put(Type.REACTOR_INIT, this); selectable = timerSelectable(); - //selectable.setDeadline(now + timeout); // TODO: this isn't in the C code... } @Override @@ -372,7 +317,7 @@ public class ReactorImpl implements Reactor { @Override public void run() { - setTimeout(3141); // TODO: eh? + setTimeout(3141); start(); while(process()) {} stop(); @@ -390,24 +335,11 @@ public class ReactorImpl implements Reactor { } return task; } - // TODO: acceptor - // TODO: connection - // TODO: acceptorClose private class TimerReadable implements Callback { @Override public void run(Selectable selectable) { - // TODO: the implication is that this will be called when the selectable is woken-up -/* - 434 static void pni_timer_readable(pn_selectable_t *sel) { - 435 char buf[64]; - 436 pn_reactor_t *reactor = pni_reactor(sel); - 437 pn_socket_t fd = pn_selectable_get_fd(sel); - 438 pn_read(reactor->io, fd, buf, 64); - 439 pni_timer_expired(sel); - 440 } - */ // TODO: this could be more elegant... new TimerExpired().run(selectable); } @@ -433,7 +365,7 @@ public class ReactorImpl implements Reactor { selectable.getChannel().close(); } catch(IOException e) { e.printStackTrace(); - // TODO: no idea what to do here... + // TODO: what to do here... } } } @@ -450,9 +382,6 @@ public class ReactorImpl implements Reactor { return sel; } - // TODO: the C code allows records to be associated with a Reactor and the Selector is get/set using that capability. - private Selector selector; - protected Selector getSelector() { return selector; } http://git-wip-us.apache.org/repos/asf/qpid-proton/blob/46b9d848/proton-j/src/main/java/org/apache/qpid/proton/reactor/impl/ReactorInternalException.java ---------------------------------------------------------------------- diff --git a/proton-j/src/main/java/org/apache/qpid/proton/reactor/impl/ReactorInternalException.java b/proton-j/src/main/java/org/apache/qpid/proton/reactor/impl/ReactorInternalException.java index d8d67ad..6dde424 100644 --- a/proton-j/src/main/java/org/apache/qpid/proton/reactor/impl/ReactorInternalException.java +++ b/proton-j/src/main/java/org/apache/qpid/proton/reactor/impl/ReactorInternalException.java @@ -1,3 +1,24 @@ +/* + * + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + * + */ + package org.apache.qpid.proton.reactor.impl; /** http://git-wip-us.apache.org/repos/asf/qpid-proton/blob/46b9d848/proton-j/src/main/java/org/apache/qpid/proton/reactor/impl/SelectorImpl.java ---------------------------------------------------------------------- diff --git a/proton-j/src/main/java/org/apache/qpid/proton/reactor/impl/SelectorImpl.java b/proton-j/src/main/java/org/apache/qpid/proton/reactor/impl/SelectorImpl.java index cf50456..28ea1ae 100644 --- a/proton-j/src/main/java/org/apache/qpid/proton/reactor/impl/SelectorImpl.java +++ b/proton-j/src/main/java/org/apache/qpid/proton/reactor/impl/SelectorImpl.java @@ -40,12 +40,12 @@ public class SelectorImpl implements Selector { private final HashSet<Selectable> error = new HashSet<Selectable>(); public SelectorImpl() throws IOException { - selector = java.nio.channels.Selector.open(); // TODO: need to ensure we close this somewhere... + selector = java.nio.channels.Selector.open(); } @Override public void add(Selectable selectable) throws IOException { - // TODO: valid for selectable to have a 'null' channel - in this case it can only expire... + // Selectable can be 'null' - if this is the case it can only ever receive expiry events. if (selectable.getChannel() != null) { selectable.getChannel().configureBlocking(false); SelectionKey key = selectable.getChannel().register(selector, 0); --------------------------------------------------------------------- To unsubscribe, e-mail: commits-unsubscr...@qpid.apache.org For additional commands, e-mail: commits-h...@qpid.apache.org
