Azure-sdk-for-js: Abort leak file descriptor

[BertholetDamien]

• Package Name: @azure/storage-blob
• Package Version: 10.3.0
• Operating system: Linux Alpine
• [x] nodejs
  
  
  
  • version: 10.15.3
  
  
• [x] typescript
  
  
  
  • version: 3.2.2
  
  

Describe the bug
When we abort the connection of a download on a blockBlobURL, there is a leak of file descriptor because the readable stream body source should be destroy.

To Reproduce
Steps to reproduce the behavior:

1. Start a simple server.

1. Check the file descriptor count of your node process with something like :
   lsof | grep socket | grep node | wc -l

2. Do a ab test on this serveur that will start a download and abort the connection

3. Check the file descriptor count after.

Some code to help :

    const aborter = Azure.Aborter.timeout(0);
    const blockBlobUrl = Azure.BlockBlobURL.fromContainerURL(this.containerUrl, filePath);
    const downloadBlockBlobResponse = await blockBlobUrl.download(aborter);

    // For test purpose
    setTimeout(() => {
        aborter.abort();
    }, 1000);

Of course, the downloadBlockBlobResponse.readableStreamBody need to be consume.

My actual work around is too destroy the readable stream body source myself :

       aborter.abort();
       // @ts-ignore
       downloadBlockBlobResponse.readableStreamBody.source.destroy();

Expected behavior
The readable stream body source should be destroy if the connection is abort.

I hope this can help !

[kurtzeborn]

Thank you for opening this issue! We are routing it to the appropriate team for follow up.

[jeremymeng]

/cc @XiaoningLiu

[XiaoningLiu]

Thanks for help debugging! What the Aborter.abort() does is to trigger aborter signal into under layer axios HTTP client, which should handle the aborted connection. (readableStreamBody is a readable stream for HTTP response body download stream, based on the connection)

It's surprised to me there will be a fd leak. Also interesting whether what will happen when we wait for longer time to see the fd drops. Still needs investigation.

[BertholetDamien]

Thank for looking into that.

If I can do anything to help you, don't hesitate.

[XiaoningLiu]

Thank for looking into that.

If I can do anything to help you, don't hesitate.

Thanks! Aborter instance is passed to under layer @azure/ms-rest-js's axios wrapper. When abort() single triggered, @azure/ms-rest-js will handle the event, and finally trigger abort event to axios. If there is not a leak in @azure/ms-rest-js, then most probably, axios should be responsible for that.

That's my first thoughts, but didn't have time for confirmation yet. You can take a look if have time.

[BertholetDamien]

Okay, I will check that when I got time !

[kurtzeborn]

Looks like the question has been addressed here. @BertholetDamien, thanks for reaching out. If you have further questions, please open another issue and we'll get right on it.

[msftbot[bot]]

Thanks for working with Microsoft on GitHub! Tell us how you feel about your experience using the reactions on this comment.

[BertholetDamien]

The problem is not solved and I don't have personnal time to check that deeper.
So for, now, I use the workaround (see the description).

[XiaoningLiu]

recently released 12.0.0-preview.3 or 10.5.0 deprecates axios and switches to node-fetch, worth a try.

[BertholetDamien]

Nice to hear ! I will check (one of these day) if this update will fix the issue and report it here.

[BertholetDamien]

Hi team,

We try the "@azure/storage-blob": "^12.0.1" and the issue is still there.

And my Workaround doesn't work anymore with these version so we had rollback to 10.4.1 with the work around.

Can you tell me if you will be able to investigate ?

Thanks a lot for your time

[XiaoningLiu]

Hi @BertholetDamien I'll try to take a look when I get time. Besides that, if possible, try to disable keep alive options and see if it works in latest version.

const pipeline = newPipeline(credential, {
      keepAliveOptions: { enable: false }
    });

[BertholetDamien]

Thanks ! I will check this option when I will have time.

Keep me posted if you look further.

[msftbot[bot]]

Thanks for the feedback! We are routing this to the appropriate team for follow-up. cc @xgithubtriage.

[msftbot[bot]]

Hi, we're sending this friendly reminder because we haven't heard back from you in a while. We need more information about this issue to help address it. Please be sure to give us your input within the next 7 days. If we don't hear back from you within 14 days of this comment the issue will be automatically closed. Thank you!

[ljian3377]

@jeremymeng
Could this be related to that recent issue? https://github.com/Azure/azure-sdk-for-js/issues/12029

[jeremymeng]

Maybe. I don't remember whether v10 dependency is still using axios or not. Either way there might be similar code.

[BertholetDamien]

@jeremymeng yes, v10 was using axios.
But v12 wasn't and I had the same problem with v12.

@ljian3377 You're seem right, theses related issues look to be the same problem.

For now on, I can't update my sdk version because of that issue. So I'm still in the v10 version.

[jeremymeng]

@BertholetDamien we fixed the issue recently in latest versions of @azure/storage-blob v12 and @azure/core-http. Could you please give it a try?

[BertholetDamien]

It's a really good new! I'm curious, can I have the related commit about that?

I'll be very happy to test, but can you let me somes days? I will give you feedback as soon as I had one.

[jeremymeng]

@BertholetDamien @azure/core-http fix: https://github.com/Azure/azure-sdk-for-js/pull/12038

[BertholetDamien]

Nice! Thank you very much for taking the time to fix this problem.

[jeremymeng]

Similar fix has been ported to https://github.com/Azure/ms-rest-js/pull/425. It should fix memory leak issue caused by the abort event handlers for storage v10.

[msftbot[bot]]

Hi, we're sending this friendly reminder because we haven't heard back from you in a while. We need more information about this issue to help address it. Please be sure to give us your input within the next 7 days. If we don't hear back from you within 14 days of this comment the issue will be automatically closed. Thank you!

[BertholetDamien]

@jeremymeng

I try the new version last week, and my kubernetes node has just get a lot of issues. So I rollback to the version 10 with my work around for the leak descriptor issue.

Honestly, I don't have time right now to give you a minimal reproductible setup or investigate in any way.

I plan to stay on the v 10 as long as I can and if I need to update one day, I will reopen an issue if the problem occur again with more information.

Thanks a lot for your work team!

[jeremymeng]

@BertholetDamien I am sorry to hear that you hit issues with the new version. Hopefully you could find some time to provide us more details in the future.

[ljian3377]

@BertholetDamien
Are you experiencing the issue with Aborter or destroy() alone.
I believe in the aborter way, the latest V12 should work as expected, releasing underlying resources. We still need to fix it for the destroy() path.

import { BlobClient } from "@azure/storage-blob";
import { AbortController } from "@azure/abort-controller";

async function main() {
    const aborter = new AbortController();
    const res = await blobClient.download(undefined, undefined, {
      abortSignal: aborter.signal,
    });
    aborter.abort();
}

See https://github.com/Azure/azure-sdk-for-js/issues/11850#issuecomment-782556735

[BertholetDamien]

@ljian3377
That was with the Aborter. The problem I had with the v12, when I test it day ago, I don't know if it's still related to the same problem (file descriptor leak) or something else. I haven't investigate, because I was in the middle of a big release, I had to manage a lot of others things.
Maybe it's not even the SDK and it was me who made a mistake with my last test (5 days ago). So I don't think investigating this is a good use of your time until I provide more information.

Did you plan to deprecated old versions at some point?

[ljian3377]

Did you plan to deprecated old versions at some point?

Yes. So we strongly encourage you to migrate to V12.

[BertholetDamien]

@ljian3377 Thanks for the warning. I will plan that in the futur.
And like I said, I will open an other ticket if needed.
Thanks a lot team!