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

outbound.deliver resolves with wrong transaction id for promises that resolve concurrently #17

Open
edward-coombes opened this issue Jul 27, 2023 · 1 comment

Comments

@edward-coombes
Copy link

edward-coombes commented Jul 27, 2023

const interfaxClient = new InterFAX()
const faxRequests = [{id:1,fileName: 'lol.pdf', faxNumber:'+999-9999-0'},{id:2,fileName:'haha.pdf',faxNumber:'+999-9999-0'}, {id:3,fileName:'jaja.pdf',faxNumber:'+999-9999-6017'}]
const concurrentFaxes = faxRequests.map(async (faxRequest) => {
        const faxResponse = await interfaxClient.outbound.deliver({
            faxNumber: faxRequest.faxNumber,
            file: faxRequest.fileName,
        });
        console.log(JSON.stringify({ txnId: faxResponse.id, id: faxRequest.id }));
})
await Promise.allSettled(concurrentFaxes)

leads to log

{"txnId":"1","id": 1}
{"txnId":"1","id":2}
{"txnId":"1","id":3}

I believe that the root cause for this is the way promises are resolved statefully via event emission in the delivery class.

 _promise(callback) {
    return new Promise((resolve, reject) => {
      this._emitter.on('resolve', (response) => {
        if (callback) { callback(null, response); }
        resolve(response);
      });
      this._emitter.on('reject', (error) => {
        if (callback) { callback(error, null); }
        reject(error);
      });
    });
  }

Hypothesis for how this occurs:
outbound.deliver(<1>) -> Promise 1 is created -> request for promise 1 is kicked off
outbound.deliver(<2>) -> Promise 2 is created -> request for promise 2 is kicked off
rest api call for promise 1 is received -> stateful emitter emits 'resolve'
Both promises are listening to the same emitter, so both promises resolve with the same response

I think this issue could be avoided by either

  1. Moving away from callbacks, and leaning into promises/async await so that an emitter is not needed in order to resolve the promises, and the emitter state causing the bug is removed
  2. Add additional state to the emitter and have it resolve/reject with a specific key unique to each call of outbound deliver, either by using the timestamp at call time of outbound.deliver, as the calls are guaranteed to happen sequentially even if the promises resolve concurrently.
  3. Create a new emitter for each instance of calling outbound.deliver, and pass that down into the deliver files method
@cwilden
Copy link

cwilden commented Aug 2, 2023

I have the same issue. When will this be fixed? @sdominInterfax

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants