SparkPost / heml

HEML is an open source markup language for building responsive email.
https://heml.io
MIT License
4.33k stars 157 forks source link

TypeError when running in parallel #44

Open andwaredev-zz opened 6 years ago

andwaredev-zz commented 6 years ago

When running heml in parallel, it seems to hit a TypeError:

TypeError {
  message: 'Cannot read property \'get\' of null',
}

Object.render (node_modules/@heml/elements/build/Style.js:104:18)
render (node_modules/@heml/render/build/renderElement.js:70:53)
exports.default (node_modules/@heml/render/build/renderElement.js:27:10)
_callee4$ (node_modules/@heml/render/build/index.js:238:74)
tryCatch (node_modules/regenerator-runtime/runtime.js:65:40)
Generator.invoke [as _invoke] (node_modules/regenerator-runtime/runtime.js:299:22)
Generator.prototype.(anonymous function) [as next] (node_modules/regenerator-runtime/runtime.js:117:21)

I threw together a little repo for easily reproducing the issue.

In case rxjs is not something you're familiar with, you can read about exactly what Observable.forkJoin does here.

Let me know if this issue is known, and if there's anything I can do to help get it resolved.

avigoldman commented 6 years ago

Hey @andrew-ware 👋 Thanks for pointing this out and for creating the repo. I don't have an answer at the moment, but I am digging into it!

theHazzard commented 6 years ago

having the same issue here

andwaredev-zz commented 6 years ago

Hey @avigoldman, I don't mean to nag, but have you made any progress on this? Is there anything you could use help with?

yarudo commented 6 years ago

I have the same issue, but it is here

node_modules/@heml/elements/build/Meta.js:44:20

tylerjbainbridge commented 6 years ago

Yup- having the same issue here. Hurting our ability to compile many emails concurrently.

andwaredev-zz commented 6 years ago

@yarudo I believe there are some structural issues with heml causing collisions between instances that share certain global props. Depending on the situation, this may throw in places other than build/Style.js. I've personally encountered the bug arising from numerous places within the build dir.

@tylerjbainbridge Unfortunately, it seems this issue may be a complex one to solve and I'm not sure the maintainers have a plan for how to do so. If you've already committed to using heml, the solution that I landed on was just serializing your heml calls, although obviously lacking the performance you'd be able to achieve in parallel.

Example of a promise serializer:

/**
 * Execute an array of promises sequentially, as opposed to in parallel.
 * This can help avoid bugs, that some libs might run into when executing on multiple instances in parallel.
 *
 * @param {[() => Promise]} funcs - an array of functions that each return a Promise
 */
const promiseSerial = funcs =>
  funcs.reduce(
    (promise, func) => promise.then(result => func().then(Array.prototype.concat.bind(result))),
    Promise.resolve([])
  );
mbkv commented 5 years ago

I was looking at the source code for heml. I came across this issue at work, and I was seeing if there's a quick way to fix it.

The real issue is located here. If you're compiling multiple emails at once, you'll come across when one (or more) of these variables becomes null. But it becomes null for every single instance of heml running.

Doing what @andwaredev suggests will help. But using a promised-based queue would make your code a lot cleaner

// This is typescript, btw
import heml from 'heml';

interface EmailJob {
  resolve: (html: string) => void;
  reject: (err: Error) => void;
  value: string;
}

class HemlWrapper {
  static protected queue: Queue<EmailJob> = new Queue<EmailJob();

  static push(heml: string) {
    return new Promise((resolve, reject) => {
      HemlWrapper.queue.enqueue({
        resolve,
        reject,
        value: heml,
      });
    });
  }

  static async run() {
    while (true) {
      const job = await HemlWrapper.queue.dequeue();
      try {
        job.resolve(await heml(job.value));
      } catch (e) {
        job.reject(e);
      }
    }
  }
}

class Queue<T> {
  protected jobs: T[] = [];
  protected resolvers: Array<(value: T) => void> = [];

  enqueue(value: T) {
    if (this.resolvers.length) {
      // We were waiting for more jobs. We can send it off immediately
      const resolver = this.resolvers.shift();
      resolver(value);
    } else {
      // We are waiting for more workers to dequeue
      this.jobs.push(value);
    }
  }

  dequeue(): Promise<T> {
    return new Promise((resolve) => {
      if (this.jobs.length) {
        // There is a job that exists. resolve early
        resolve(this.jobs.shift());
      } else {
        // There's nothing to do. store to be resolved later
        this.resolvers.push(resolve);
      }
    });
  }
}

// Usage:
HemlWrapper.run();

for (let i = 0; i < 100; i++) {
  HemlWrapper.push(someHemlFile)
    .then((heml) => {
      console.log('I just compiled someHemlFile!');
    }, (err) => {
      console.error('There was an error in compiling someHemlFile');
    });
}

I've used this same Queue in a few of my projects. It's incredibly useful for stuff like rate limiting. But there is still a problem with this. You're effectively limited to having only one heml request at a time. So you might be a little slower, depending how many Node API calls Heml has