Failed to fetch instance destinations. (Timed out after 10000ms)

LaxOn commented 3 years ago

Description of erroneous behaviour

We have a Node.js CAP application edom-retailer-core. In MDIClient.js, we are making a request via destination connecting to MdiDestination.

// package.json
"MdiDestination": {
   "kind": "rest",
   "credentials": {
      "destination": "mdi-dest",
      "requestTimeout": 1000000

The API in the destination retrieves 2000 items at a time and if it has more data, it will provide a deltaToken that we use to make the same call but with the deltaToken to retrieve the next 2000 items. We repeat this till all the items are retrieved. Currently our API has 19000+ items.

// MDIClient.js
const MDIDest = await cds.connect.to('MdiDestination');
const logRequest = new cds.Request();
const baseQuery = `GET /log/sap.odm.businesspartner.BusinessPartner`;

let moreData = true;
while (moreData) {
   if (deltaToken !== false) {
     logRequest.query = `${baseQuery}?since=${deltaToken}`;
   }
   const response = await MDIDest.tx(logRequest).run(logRequest.query);

   ...

   if (!response.hasMoreEvents) {
      moreData = false;
   }
}

The request is successful for the first 6-9 iterations but throws an error afterwards:

Error: Timed out after 10000ms
    at buildError (/mnt/c/Users/C5315270/Desktop/projects/edom-retailer-core/node_modules/opossum/lib/circuit.js:710:17)
    at Timeout.<anonymous> (/mnt/c/Users/C5315270/Desktop/projects/edom-retailer-core/node_modules/opossum/lib/circuit.js:511:29)
    at listOnTimeout (internal/timers.js:554:17)
    at processTimers (internal/timers.js:497:7) {
  cause: ErrorWithCause: Failed to fetch instance destinations.
      at new ErrorWithCause (/mnt/c/Users/C5315270/Desktop/projects/edom-retailer-core/node_modules/@sap-cloud-sdk/util/dist/error-with-cause.js:31:16)
      at /mnt/c/Users/C5315270/Desktop/projects/edom-retailer-core/node_modules/@sap-cloud-sdk/core/dist/connectivity/scp-cf/destination/destination-service.js:116:27
      at runNextTicks (internal/process/task_queues.js:62:5)
      at listOnTimeout (internal/timers.js:523:9)
      at processTimers (internal/timers.js:497:7)
      at async Promise.all (index 0)

I attached the console log and console error below. log.txt error.txt

Detailed steps to reproduce

For example (→ replace by appropriate ones for your case):

git clone https://github.wdf.sap.corp/c4u/edom-retailer-core.git

git checkout feat/25-mdiclient-outbound

npm install

cds deploy

cds run

In Postman, make a GET request to http://localhost:4004/api/alpha/mdiclient/MDIClient

Details about your project

edom-retailer	https://github.wdf.sap.corp/c4u/c4u-cn-edom-retailer
CAP Java Runtime	version
OData Version	version
@sap/cds-compiler	2.1.6
@sap/cds-dk	4.0.7

Used Versions:

node version via node -v: v12.22.1
npm version via npm -v: 6.14.12
SAP Cloud SDK version you used as dependency
- "@sap-cloud-sdk/core": "^1.41.0",

artemkovalyov commented 3 years ago

Hi @LaxOn,

Thanks for bringing this up. We'll take a look into this shortly and get back to you with ideas.

artemkovalyov commented 3 years ago

@LaxOn, we'll have to provide an API to override the default timeout. Can you please share your deadline to have this solved? We'll also communicate with CAP to make sure their configuration for destinations is properly passed down to the Cloud SDK APIs. cc @jjtang1985 @marikaner

LaxOn commented 3 years ago

Thanks @artemkovalyov for the quick response! We need it by the 2105 release (May 30, 2021). Thanks a lot!

jjtang1985 commented 3 years ago

Hi @LaxOn ,

thanks for sharing more information about the timeline. The timeout configuration ticket is planned to be started in the next week. (not guaranteed).

Please keep in mind, you might NOT meet your deadline even with the new configuration because of the reasons:

we assume having a longer timeout can help in your case, which might not be the case
we need a new stable release for the configuration instead of using canary version, because you are not consuming the SDK directly, and CAP needs stable version.
CAP is able to use our new configurations

Best regards, Junjie

marikaner commented 3 years ago

Hey @LaxOn,

I am currently investigating this issue more closely and found some things that might be of interest for you:

The first issue in the given logs actually states that a "Possible EventEmitter memory leak [was] detected". I found that, although I wasn't able to reproduce this, it is likely happening because of a bug in the SAP Cloud SDK. We will fix this as soon as we can.
I was able to reproduce the timeout issues by retrieving the same destination over and over. I have a few follow up questions and ideas for you. If I am not mistaken you call this line: const dest = await cloudSDK.getDestination('mdi-dest'); (or previously this line const MDIDest = await cds.connect.to('MdiDestination');) for every incoming request. However, the result of this request is always the same destination. Therefore, in theory it would suffice to make this request once and reuse the destination for all subsequent requests.

If this is correct, you might be able work around this whole issue by requesting the destination once initially and reusing it for all subsequent requests. It would be great if you could try this approach and let us know if it solves your issue. I am afraid there might be a second, similar issue with the XSUAA service down the line. Still under the assumption that this is correct, it would be even better to be able to use the SAP Cloud SDK's caching mechanisms for this. We realized, that we have potential for improvement here. As of today, the SAP Cloud SDK's caching cannot be used by CAP.

That said, I believe increasing the timeout might also solve your problem, however there are a few issues with this:

If my assumptions above are correct, it is a waste of resources to make the same requests over and over.
We are currently running into this timeout, because we are using a circuit breaker that shall protect the underlying systems. This circuit breaker is overarching all tenants and can therefore not be configured individually. It can however be turned off. That functionality again (similar to the caching) is not exposed in a manner, that it could be used with CAP (as of today).

We will discuss options internally. In the meantime it would be very helpful to know if my proposal above helps with your problem.

marikaner commented 3 years ago

Hey @LaxOn,

btw, you can enable the SDK caching if you transform this line: const dest = await cloudSDK.getDestination('mdi-dest'); to this: const dest = await cloudSDK.getDestination('mdi-dest', { useCache: true });

LaxOn commented 3 years ago

Thanks for looking into this. I'll ping the CAP team to take a look at this as they're the one directly using cloudSDK.getDestination(). We would only be using the wrapper they provide.

I just noticed the MDIClient.js implementation I linked in the main post changed to the workaround we just implemented. I updated it to the original implementation that we had an issue with.

johannes-vogel commented 3 years ago

Hi @marikaner and @LaxOn,

within CAP we only use the executeHttpRequest method, that the Cloud SDK provides. Is caching possible with that as well?

marikaner commented 3 years ago

Thanks for looking into this. I'll ping the CAP team to take a look at this as they're the one directly using cloudSDK.getDestination(). We would only be using the wrapper they provide.

I just noticed the MDIClient.js implementation I linked in the main post changed to the workaround we just implemented. I updated it to the original implementation that we had an issue with.

But did it work with the workaround? This might be helpful for an intermediate solution.

marikaner commented 3 years ago

Hi @marikaner and @LaxOn,

within CAP we only use the executeHttpRequest method, that the Cloud SDK provides. Is caching possible with that as well?

This is currently not yet possible, but we created a ticket for this. Theoretically you could already use it by applying the mentioned workaround from above.

marikaner commented 3 years ago

@LaxOn It would be very helpful for us to understand whether enabling the cache solves your problem. If so we would consider to prioritize the exposure of the caching capabilities to executeHttpRequest.

LaxOn commented 3 years ago

Hi @marikaner

Thanks for looking into this. I'll ping the CAP team to take a look at this as they're the one directly using cloudSDK.getDestination(). We would only be using the wrapper they provide. I just noticed the MDIClient.js implementation I linked in the main post changed to the workaround we just implemented. I updated it to the original implementation that we had an issue with.

But did it work with the workaround? This might be helpful for an intermediate solution.

Our workaround is to use axios and manually building an http request. That workaround did not use executeHttpRequest so I don't know if enabling the cache would for sure solve our problem. Any thoughts @johannes-vogel ?

johannes-vogel commented 3 years ago

Hi @LaxOn,

Our workaround is to use axios and manually building an http request. That workaround did not use executeHttpRequest so I don't know if enabling the cache would for sure solve our problem. Any thoughts @johannes-vogel ?

Are you using cloudSDK.getDestination('mdi-dest', { useCache: true }) to retrieve the actual information of your endpoint and then use axios to retrieve the actual content?

If yes and if I understand @marikaner correctly, they would prioritize the destination caching feature for executeHttpRequest which is used by @sap/cds under the hood.

marikaner commented 3 years ago

@johannes-vogel, yes, you understand correctly ;)

LaxOn commented 3 years ago

@johannes-vogel We do use cloudSDK.getDestination('mdi-dest', { useCache: true }) and axios for our workaround but we want to use our original implementation:

const MDIDest = await cds.connect.to('MdiDestination');
const logRequest = new cds.Request();
const baseQuery = `GET /log/sap.odm.businesspartner.BusinessPartner`;
const response = await MDIDest.tx(logRequest).run(logRequest.query);

as we are under the impression that this is the CAP way of sending requests where we don't explicitly use cloudSDK.getDestination('mdi-dest', { useCache: true }) and axios. Would destination caching feature for executeHttpRequest solve our issue with our original implementation as well?

marikaner commented 3 years ago

@LaxOn If cloudSDK.getDestination('mdi-dest', { useCache: true }) fixes your issue, then yes, it could solve your problem as well. I take it that the workaround in fact does fix the issue?

marikaner commented 3 years ago

FYI, the erroneous use of the circuit breaker is fixed and will be released with the next version. This, however should not influence your issue.

LaxOn commented 3 years ago

Yes,

@LaxOn If cloudSDK.getDestination('mdi-dest', { useCache: true }) fixes your issue, then yes, it could solve your problem as well. I take it that the workaround in fact does fix the issue?

Yes, our workaround currently works. Please let the CAP team know once the update with the fix is released. Thanks!

johannes-vogel commented 3 years ago

@marikaner We currently think of applying this workaround also in CAP. However, I have a short question regarding cloudSDK.getDestination('mdi-dest', { useCache: true }). How would that behave if this is a multi tenant destination that is maintained in the subscriber accounts?

marikaner commented 3 years ago

@johannes-vogel That depends on the isolation strategy. The default is "Tenant", which means that this is tenant aware. I hope that explains it?

We have discussed how to proceed on this, today. We were wondering whether the approach of calling getDestination is good enough and does not require adding the DestinationOptions to the parameters of executeHttpRequest. The reason is, that we don't want to overcomplicate the signature of this function and getDestination already can handle this. I remember that in the initial API design, we actually discussed this and decided to not add it for the mentioned reasons.

Let me know whether you think it would suffice to improve the documentation on this or whether you still would like to have this feature.

marikaner commented 3 years ago

As discussed offline, the SDK API will remain as is, but we will improve the documentation on these possibilities.

LaxOn commented 3 years ago

@marikaner Wanted to further clarify the multitenant scenario:

Currently the behavior we have is that const dest = await cloudSDK.getDestination('mdi-dest', { useCache: true }); connects to the destination service in the provider subaccount.

What do we need to do so it instead connects to the destination service in the consumer subaccount?

LaxOn commented 3 years ago

Hi @marikaner just wanted to follow up on this.

I've tried running const dest = await cloudSDK.getDestination('mdi-dest',{ useCache: true, isolationStrategy: "Tenant"}) and this connects to the provider subaccount still. How do we connect to the consumer subaccount?

LaxOn commented 3 years ago

I'm able to connect to the consumer subaccount by providing a userJwt. Thank you! await cloudSDK.getDestination('mdi-dest',{ useCache: true, isolationStrategy: "Tenant", userJwt:jwt})

artemkovalyov commented 3 years ago

Thanks for the update, @LaxOn. Don't hesitate to come to us if you need help with anything else.

SAP / cloud-sdk-js