Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

TODO/XXX/FIXME comments in lib directory #4642

Closed
Trott opened this issue Jan 12, 2016 · 12 comments
Closed

TODO/XXX/FIXME comments in lib directory #4642

Trott opened this issue Jan 12, 2016 · 12 comments
Labels
lib / src Issues and PRs related to general changes in the lib or src directory.

Comments

@Trott
Copy link
Member

Trott commented Jan 12, 2016

Ref: #264

There are 60+ TODO, XXX, and FIXME comments in the lib directory. It would be great to either remove them from the code (if they are no longer valid or at least not particularly high value), or get issues opened for them, or just get whatever it is they are addressing addressed. Here they are as of this writing.

lib/_debug_agent.js:

  // Just to spin-off events
  // TODO(indutny): Figure out why node.cc isn't doing this
  setImmediate(function() {
  });

lib/_debugger.js:

  var self = this;

  // TODO: We have a cache of handle's we've already seen in this.handles
  // This can be used if we're careful.
  var req = {
...

// reqBacktrace(cb)
// TODO: from, to, bottom
Client.prototype.reqBacktrace = function(cb) {
  this.req({ command: 'backtrace', arguments: { inlineRefs: true } }, cb);
...

// reqSetExceptionBreak(type, cb)
// TODO: from, to, bottom
Client.prototype.reqSetExceptionBreak = function(type, cb) {
  this.req({
...
  this.pause();

  // XXX Need to figure out why we need this delay
  setTimeout(function() {

...
  self.killChild();

  // XXX need to wait a little bit for the restart to work?
  setTimeout(function() {
    self.trySpawn();

lib/_http_client.js:

      socket.removeListener('error', socketErrorListener);

      // TODO(isaacs): Need a way to reset a stream to fresh state
      // IE, not flowing, and not explicitly paused.
      socket._readableState.flowing = null;

lib/_http_common.js:

}

// XXX This is a mess.
// TODO: http.Parser should be a Writable emits request/response events.
function parserOnBody(b, start, len) {
  var parser = this;
...
// Free the parser and also break any links that it
// might have to any other things.
// TODO: All parser data should be attached to a
// single object, so that it can be easily cleaned
// up by doing `parser.data = {}`, which should

lib/_http_incoming.js:

  Stream.Readable.call(this);

  // XXX This implementation is kind of all over the place
  // When the parser emits body chunks, they go in this list.
  // _read() pulls them out, and when it finds EOF, it ends.

lib/_http_server.js:

  parser[kOnExecute] = onParserExecute;

  // TODO(isaacs): Move all these functions out of here
  function socketOnError(e) {
    // Ignore further errors
...
        var bodyHead = d.slice(bytesParsed, d.length);

        // TODO(isaacs): Need a way to reset a stream to fresh state
        // IE, not flowing, and not explicitly paused.
        socket._readableState.flowing = null;

lib/_stream_writable.js:

function writeAfterEnd(stream, cb) {
  var er = new Error('write after end');
  // TODO: defer error events consistently everywhere, not just the cb
  stream.emit('error', er);
  process.nextTick(cb, er);
...

    // count the one we are adding, as well.
    // TODO(isaacs) clean this up
    state.pendingcb++;
    state.lastBufferedRequest = null;

lib/_tls_common.js:

    c.infoAccess = {};

    // XXX: More key validation?
    info.replace(/([^\n:]*):([^\n]*)(?:\n|$)/g, function(all, key, val) {
      if (key === '__proto__')

lib/_tls_legacy.js:

  // side.
  //
  // TODO(indutny): Remove magic number, use watermark based limits
  if (!this._resumingSession &&
      this._opposite._internallyPendingBytes() < 128 * 1024) {
...

CryptoStream.prototype._read = function read(size) {
  // XXX: EOF?!
  if (!this.pair.ssl) return this.push(null);

lib/_tls_wrap.js:

      return cb(new Error('Socket is closed'));

    // TODO(indutny): eventually disallow raw `SecureContext`
    if (context)
      self._handle.sni_context = context.context || context;
...
  // lib/net.js expect this value to be non-zero if write hasn't been flushed
  // immediately
  // TODO(indutny): rewise this solution, it might be 1 before handshake and
  // represent real writeQueueSize during regular writes.
  ssl.writeQueueSize = 1;
...
};

// TODO: support anonymous (nocert) and PSK

lib/buffer.js:

// TODO(trevnorris): fix these checks to follow new standard
// write(string, offset = 0, length = buffer.length, encoding = 'utf8')
var writeWarned = false;
...
    }

  // XXX legacy write(string, encoding, offset, length) - remove in v0.13
  } else {
    writeWarned = internalUtil.printDeprecationMessage(writeMsg, writeWarned);
...


// TODO(trevnorris): currently works like Array.prototype.slice(), which
// doesn't follow the new standard for throwing on out of range indexes.
Buffer.prototype.slice = function slice(start, end) {

lib/cluster.js:

  this.errno = 0;

  // FIXME(bnoordhuis) Polymorphic return type for lack of a better solution.
  var rval;
  if (addressType === 'udp4' || addressType === 'udp6')
...
      var out = {};
      self.handle.getsockname(out);
      // TODO(bnoordhuis) Check err.
      send(null, { sockname: out }, null);
    } else {
...
  cluster.settings = {};

  // XXX(bnoordhuis) Fold cluster.schedulingPolicy into cluster.settings?
  var schedulingPolicy = {
    'none': SCHED_NONE,
...

  if (schedulingPolicy === undefined) {
    // FIXME Round-robin doesn't perform well on Windows right now due to the
    // way IOCP is wired up. Bert is going to fix that, eventually.
    schedulingPolicy = (process.platform === 'win32') ? SCHED_NONE : SCHED_RR;
...
  // Keyed on address:port:etc. When a worker dies, we walk over the handles
  // and remove() the worker from each one. remove() may do a linear scan
  // itself so we might end up with an O(n*m) operation. Ergo, FIXME.
  const handles = require('internal/cluster').handles;

...
    var key = message.key;
    function listen(backlog) {
      // TODO(bnoordhuis) Send a message to the master that tells it to
      // update the backlog size. The actual backlog should probably be
      // the largest requested size by any worker.
...
    }

    // XXX(bnoordhuis) Probably no point in implementing ref() and unref()
    // because the control channel is going to keep the worker alive anyway.
    function ref() {

lib/console.js:

Console.prototype.trace = function trace() {
  // TODO probably can to do this better with V8's debug object once that is
  // exposed.
  var err = new Error();

lib/dns.js:

function errnoException(err, syscall, hostname) {
  // FIXME(bnoordhuis) Remove this backwards compatibility nonsense and pass
  // the true error to the user. ENOTFOUND is not even a proper POSIX error!
  if (err === uv.UV_EAI_MEMORY ||

lib/domain.js:

  // ignore errors on disposed domains.
  //
  // XXX This is a bit stupid.  We should probably get rid of
  // domain.dispose() altogether.  It's almost always a terrible
  // idea.  --isaacs

lib/internal/child_process.js:

  channel.buffering = false;
  channel.onread = function(nread, pool, recvHandle) {
    // TODO(bnoordhuis) Check that nread > 0.
    if (pool) {
      jsonBuffer += decoder.write(pool);
...
      process.nextTick(callback, ex);
    } else {
      this.emit('error', ex);  // FIXME(bnoordhuis) Defer to next tick.
    }
    return false;
...
        process.nextTick(callback, ex);
      } else {
        this.emit('error', ex);  // FIXME(bnoordhuis) Defer to next tick.
      }
    }

lib/internal/repl.js:

module.exports.createInternalRepl = createRepl;

// XXX(chrisdickinson): The 15ms debounce value is somewhat arbitrary.
// The debounce is to guard against code pasted into the REPL.
const kDebounceHistoryMS = 15;
...
    opts.historySize = historySize;
  } else {
    // XXX(chrisdickinson): set here to avoid affecting existing applications
    // using repl instances.
    opts.historySize = 1000;

lib/module.js:

};

// TODO(bnoordhuis) Unused, remove in the future.
Module.requireRepl = internalUtil.deprecate(function() {
  return NativeModule.require('internal/repl');

lib/net.js:

// up.
function onSocketEnd() {
  // XXX Should not have to do as much crap in this function.
  // ended should already be true, since this is called *after*
  // the EOF errno and onread has eof'ed
...
  er.code = 'EPIPE';
  var self = this;
  // TODO: defer error events consistently everywhere, not just the cb
  self.emit('error', er);
  if (typeof cb === 'function') {
...
    var out = {};
    var err = this._handle.getpeername(out);
    if (err) return {};  // FIXME(bnoordhuis) Throw?
    this._peername = out;
  }
...
    var out = {};
    var err = this._handle.getsockname(out);
    if (err) return {};  // FIXME(bnoordhuis) Throw?
    this._sockname = out;
  }
...

function connect(self, address, port, addressType, localAddress, localPort) {
  // TODO return promise from Socket.prototype.connect which
  // wraps _connectReq.

...

  // If host is an IP, skip performing a lookup
  // TODO(evanlucas) should we hot path this for localhost?
  var addressType = exports.isIP(host);
  if (addressType) {
...

    // It's possible we were destroyed while looking this up.
    // XXX it would be great if we could cancel the promise returned by
    // the look up.
    if (!self._connecting) return;
...

  // Update handle if it was wrapped
  // TODO(indutny): assert that the handle is actually an ancestor of old one
  handle = self._handle;

...
    // port before calling listen().
    //
    // FIXME(bnoordhuis) Doesn't work for pipe handles, they don't have a
    // getsockname() method. Non-issue for now, the cluster module doesn't
    // really support pipes anyway.
...
    var out = {};
    this._handle.getsockname(out);
    // TODO(bnoordhuis) Check err and throw?
    return out;
  } else if (this._pipeName) {

lib/path.js:

  var f = win32SplitPath(path)[2];
  // TODO: make this comparison case-insensitive on windows?
  if (ext && f.substr(-1 * ext.length) === ext) {
    f = f.substr(0, f.length - ext.length);

lib/querystring.js:

  }

  // TODO support returning arbitrary buffers.

  return out.slice(0, outIndex - 1);

lib/readline.js:

    if (err) {
      // XXX Log it somewhere?
      return;
    }

lib/repl.js:

  // save the line so I can do magic later
  if (cmd) {
    // TODO should I tab the level?
    const len = self.lines.level.length ? self.lines.level.length - 1 : 0;
    self.lines.push('  '.repeat(len) + cmd);
...
    // if the REPL still has a bufferedCommand and
    // self.lines.level.length === 0
    // TODO? keep a log of level so that any syntax breaking lines can
    // be cleared on .break and in the case of a syntax error?
    // TODO? if a log was kept, then I could clear the bufferedComand and
    // eval these lines and throw the syntax error
  } else {

lib/string_decoder.js:

// characters. CESU-8 is handled as part of the UTF-8 encoding.
//
// @TODO Handling all encodings inside a single object makes it very difficult
// to reason about this code, so it should be split up in the future.
// @TODO There should be a utf8-strict encoding that rejects invalid UTF-8 code
// points as used by CESU-8.
const StringDecoder = exports.StringDecoder = function(encoding) {
...
    // See http://en.wikipedia.org/wiki/UTF-8#Description

    // 110XXXXX
    if (i === 1 && c >> 5 === 0x06) {
      this.charLength = 2;
...
    }

    // 1110XXXX
    if (i <= 2 && c >> 4 === 0x0E) {
      this.charLength = 3;
...
    }

    // 11110XXX
    if (i <= 3 && c >> 3 === 0x1E) {
      this.charLength = 4;

lib/url.js:

    // http://a@b?@c => user:a host:b path:/?@c

    // v0.12 TODO(isaacs): This is not quite how Chrome does things.
    // Review our test case against browsers more comprehensively.

/cc @indutny @isaacs @trevnorris @bnoordhuis @chrisdickinson @evanlucas

@Trott Trott added help wanted Issues that need assistance from volunteers or PRs that need help to proceed. good first issue Issues that are suitable for first-time contributors. lib / src Issues and PRs related to general changes in the lib or src directory. labels Jan 12, 2016
@thefourtheye
Copy link
Contributor

X linking #4641

@chrisdickinson
Copy link
Contributor

I think my use of XXX can probably be switched to NB or changed to a plain comment.

@indutny
Copy link
Member

indutny commented Jan 12, 2016

  • _debug_agent.js I know why this is happening, but can't say that I like it. It should probably say setImmediate() should not be necessary to trigger 'nextTick' callbacks
  • _tls_legacy.js - who cares about legacy, could be removed
  • _tls_wrap.js
    • eventually disallow raw 'SecureContext' should be transformed into github issue
    • rewise... should be transformed into github issue
  • net.js - github issue

@kohei-takata
Copy link
Contributor

I want to fix lib/readline.js.
Now, readline returns nothing and no logs when callback method returns err.
I think adding line of output some log is good.
What do everyone think?
Should I open an issue about it?

@Fishrock123
Copy link
Contributor

@kohei-takata Sure. I suggest using util.debuglog() for it. :)

@kohei-takata
Copy link
Contributor

@Fishrock123
Thanks!
OK, I will change XXX in lib/readline.js to use util.debuglog() and open a PR 😄

@trevnorris
Copy link
Contributor

Can remove both my TODO.

pgeiss added a commit to pgeiss/node that referenced this issue Jan 16, 2016
Original committer stated they were safe to remove in issue post.
Ref: Issue nodejs#4642
cjihrig pushed a commit that referenced this issue Jan 17, 2016
Refs: #4642
PR-URL: #4719
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
evanlucas pushed a commit that referenced this issue Jan 18, 2016
Refs: #4642
PR-URL: #4719
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
MylesBorins pushed a commit that referenced this issue Jan 28, 2016
Refs: #4642
PR-URL: #4719
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
MylesBorins pushed a commit that referenced this issue Feb 11, 2016
Refs: #4642
PR-URL: #4719
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
MylesBorins pushed a commit to MylesBorins/node that referenced this issue Feb 11, 2016
Refs: nodejs#4642
PR-URL: nodejs#4719
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
MylesBorins pushed a commit to MylesBorins/node that referenced this issue Feb 13, 2016
Refs: nodejs#4642
PR-URL: nodejs#4719
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
MylesBorins pushed a commit to MylesBorins/node that referenced this issue Feb 15, 2016
Refs: nodejs#4642
PR-URL: nodejs#4719
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
@ghaiklor
Copy link
Contributor

I want to help, but I need a mentor, please. I don't understand some issues and what exactly I need to do. Will be great if we can get in touch via Gitter or Skype.

@thefourtheye
Copy link
Contributor

@ghaiklor you can talk to the devs in IRC.

scovetta pushed a commit to scovetta/node that referenced this issue Apr 2, 2016
Refs: nodejs#4642
PR-URL: nodejs#4719
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
AdriVanHoudt added a commit to AdriVanHoudt/node that referenced this issue Sep 26, 2016
jasnell pushed a commit that referenced this issue Oct 7, 2016
Refs: #4642
PR-URL: #8575
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Сковорода Никита Андреевич <[email protected]>
Reviewed-By: James M Snell <[email protected]>
jasnell pushed a commit that referenced this issue Oct 10, 2016
Refs: #4642
PR-URL: #8575
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Сковорода Никита Андреевич <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@kalrover
Copy link
Contributor

I would like to help you to fix lib/_http_client.js, instead of flowing how can i reset stream ?

@Trott Trott removed the good first issue Issues that are suitable for first-time contributors. label Apr 25, 2017
@jasnell
Copy link
Member

jasnell commented May 30, 2017

@Trott ... any reason to keep this open?

@Trott
Copy link
Member Author

Trott commented May 30, 2017

(copy/pasted from a related issue)

@jasnell I'm OK with closing this and the other TODO/XXX/FIXME issues. If any of those items are things that really ought to be fixed (rather than a wishlist or a "will fix after Magical Feature X is available"), a separate issue should be opened anyway because it's just getting lost in these out-of-date tracking issues.

While I think this issue is superfluous personally, anyone else should feel free to re-open (if GitHub permits them to) or comment requesting this be re-opened.

@Trott Trott closed this as completed May 30, 2017
@Trott Trott removed help wanted Issues that need assistance from volunteers or PRs that need help to proceed. mentor-available labels May 30, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lib / src Issues and PRs related to general changes in the lib or src directory.
Projects
None yet
Development

No branches or pull requests

10 participants