UDP sender : Buffer overflow on flush end

[mathieu-renard]

This issue is related to #150 and #124

I have a buffer overflow when many spans were appended during flush with UDP sender, this causes an EMSGSIZE error.

After a lot of research, I found a TODO in : https://github.com/jaegertracing/jaeger-client-node/blob/master/src/reporters/udp_sender.js

"TODO theoretically we can have buffer overflow here too, if many spans were appended during flush()"

append(span: any, callback?: SenderCallback): void {
    const { err, length } = this._calcSpanSize(span);
    if (err) {
      SenderUtils.invokeCallback(callback, 1, `error converting span to Thrift: ${err}`);
      return;
    }
    const spanSize = length;
    if (spanSize > this._maxSpanBytes) {
      SenderUtils.invokeCallback(
        callback,
        1,
        `span size ${spanSize} is larger than maxSpanSize ${this._maxSpanBytes}`
      );
      return;
    }

    if (this._totalSpanBytes + spanSize <= this._maxSpanBytes) {
      this._batch.spans.push(span);
      this._totalSpanBytes += spanSize;
      if (this._totalSpanBytes < this._maxSpanBytes) {
        // still have space in the buffer, don't flush it yet
        SenderUtils.invokeCallback(callback, 0);
        return;
      }
      // buffer size === this._maxSpanBytes
      this.flush(callback);
      return;
    }

    this.flush((numSpans: number, err?: string) => {
      // TODO theoretically we can have buffer overflow here too, if many spans were appended during flush()
      this._batch.spans.push(span);
      this._totalSpanBytes += spanSize;
      SenderUtils.invokeCallback(callback, numSpans, err);
    });
  }

During flush this._batch.spans and this._totalSpanBytes continue to be incremented when new spans append, so when flush end the totalSpanBytes (+spanSize) may be greater than this._maxSpanBytes and lead to an EMSGSIZE error when the next span append.

I made a quick fix, maybe not the best way to fix this and it could have side effects, but it works for me and I don't have EMSGSIZE error anymore :

append(span: any, callback?: SenderCallback): void {
    const { err, length } = this._calcSpanSize(span);
    if (err) {
      SenderUtils.invokeCallback(callback, 1, `error converting span to Thrift: ${err}`);
      return;
    }
    const spanSize = length;
    if (spanSize > this._maxSpanBytes) {
      SenderUtils.invokeCallback(
        callback,
        1,
        `span size ${spanSize} is larger than maxSpanSize ${this._maxSpanBytes}`
      );
      return;
    }

    if (this._totalSpanBytes + spanSize <= this._maxSpanBytes) {
      this._batch.spans.push(span);
      this._totalSpanBytes += spanSize;
      if (this._totalSpanBytes < this._maxSpanBytes) {
        // still have space in the buffer, don't flush it yet
        SenderUtils.invokeCallback(callback, 0);
        return;
      }
      // buffer size === this._maxSpanBytes
      this.flush(callback);
      return;
    }

    this.flushWithPendingSpan(span, spanSize, callback);
  }

  flushWithPendingSpan(span, spanSize, callback){
    this.flush((numSpans, err) => {
      //Test totalSpanBytes before push
      if(this._totalSpanBytes + spanSize <= this._maxSpanBytes) {
        this._batch.spans.push(span);
        this._totalSpanBytes += spanSize;

        if (this._totalSpanBytes < this._maxSpanBytes) {
          // still have space in the buffer, don't flush it yet
          SenderUtils.invokeCallback(callback, numSpans, err);
          return;
        }
        // buffer size === this._maxSpanBytes
        this.flush(callback);
      }else{
        //Not enough space so we call recursively flushWithPendingSpan
        this.flushWithPendingSpan(span, spanSize, callback);
      }
    });
  }

With a constant heavy load the pending span may never be flush or only after multiple recursive call. Maybe it would be better to send the pending span alone to avoid recursive call.

[yurishkuro]

could you open a PR to make it easier to see the changes?

[mathieu-renard]

PR opened, but just for code comparison.

[ebergama]

@mathieu-renard thanks for taking care of this! any plans on merging it? It's bothering to me too

[yurishkuro]

Unfortunately, #395 is not in a state where it can be merged: no DCO, build is broken, and no unit tests.