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

[Event Hubs] Add support for token cancellation in send requests #3323

Merged

Conversation

ShivangiReja
Copy link
Member

@ShivangiReja ShivangiReja commented May 30, 2019

Allow users to cancel a send request as described in #2641 (comment) for send().

This PR uses addEventListener and removeEventListener of Aborter class.

Added onAborted listener using addEventListener method that listens aborted request and throws error when user calls aborter.abort(). It means that at any time if user wants to cancel the send request they can create an instance of Aborter and call abort().

example:

async function main(): Promise<void> {
  const client = EventHubClient.createFromConnectionString(connectionString, eventHubName);
  const partitionIds = await client.getPartitionIds();
  const aborter = Aborter.none;
  const sender = client.createSender({ partitionId: partitionIds[0] });
  const events: EventData[] = [];

  for (let index = 0; index <= 1000; index++) {
    events.push({ body: `Hello world...!!!${index}` });
  }
  console.log("Sending batch events...");
  try {
    sender.send(events, { cancellationToken: aborter });
    await delay(2000);
    aborter.abort();
  } finally {
    await client.close();
  }
}

@ShivangiReja ShivangiReja added Client This issue points to a problem in the data-plane of the library. Event Hubs labels May 30, 2019
@ShivangiReja ShivangiReja requested a review from ramya-rao-a May 30, 2019 23:22
@ShivangiReja ShivangiReja self-assigned this May 30, 2019
`address "${this.address}", cannot send the message. opreation ` +
`aborted.`;
log.error(desc);
throw new Error(desc);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since rest of the "error" scenarios in this file are using the "reject the promise" pattern, lets do the same here instead of throwing the error.

return reject(new Error(desc))

Also, the current messaging implies that we were not able to send the message, and so it is aborted. Instead the message should be that the send operation has been cancelled by the user

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have updated this with 710ddd9

Copy link
Member Author

@ShivangiReja ShivangiReja May 31, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reverted reject(new Error(desc)) -> throw new Error(err)

If we reject the promise, it continues the retry operation with this error:
image
And this error is retryable, so program continues the operation and doesn't exit.

If we throw the error, program works as expcted.

Copy link
Contributor

@ramya-rao-a ramya-rao-a left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When the send operation is successful, we should remove the listener that we added here, else we will soon hit the limit for the number of listeners an event can have. Can you check how the Aborter class allows us to do that?

@ramya-rao-a
Copy link
Contributor

We might need to use addEventListener and removeEventListener methods instead of onAbort

@ramya-rao-a ramya-rao-a merged commit 6438dab into Azure:event-hubs-track2 May 31, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Client This issue points to a problem in the data-plane of the library. Event Hubs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants