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

S3 connection leak with PrestoS3FileSystem #375

Open
shubhamtagra opened this issue May 6, 2020 · 1 comment
Open

S3 connection leak with PrestoS3FileSystem #375

shubhamtagra opened this issue May 6, 2020 · 1 comment

Comments

@shubhamtagra
Copy link

S3 connections can leak in the case CachingInputStream is reading data via DirectReadRequestChain. The general flow of events in such read is:

  • CachingInputStream gets a read
  • It then creates a DirectReadReadRequestChain (along with other Chains), passing a PrestoS3InputStream object to it
  • It then executes this DirectReadReadRequestChain in the new thread

Now if query is cancelled while DirectReadReadRequestChain is in progress then following happens on presto Driver thread:

  • CachingInputStream.readInternal gets InterruptedException on Future.get()
  • CachingInputStream marks cancelled on DirectReadReadRequestChain in response and returns by rethrowing this exception
  • Eventually recordreader closes the CachingInputStream
  • CachingInputStream.close calls close() on PrestoS3InputStream object passed to DirectReadReadRequestChain
  • PrestoS3InputStream.close closes s3Stream if it is not null

What can happen in parallel to this PrestoS3InputStream.close, in the thread executing DirectReadReadRequestChain:

  • DirectReadReadRequestChain execution might have crossed over the check for cancelled in current iteration and issued read on PrestoS3InputStream
  • This read on PrestoS3InputStream could need to close and reopen existing s3Stream via PrestoS3InputStream.seekStream due to long jump in read position
  • This method does it as follows: close s3Stream -> set s3Stream to null -> open new s3Stream

A race between these two threads can cause PrestoS3InputStream.close to execute (in Driver thread) when s3Stream is set null by seekStream (in DirectReadReadRequestChain thread).
Which means the underlying s3Stream created by seekStream in DirectReadReadRequestChain thread never gets closed as DirectReadReadRequestChain expects CachingInputStream
to close the underlying streams during its close().

The fix for this is to let the Chains open and close stream themselves. Tried this and could see no connections are leaked.

@shubhamtagra
Copy link
Author

The fix for this is to let the Chains open and close stream themselves: while this solves the issue, this can have impact on performance when streaming reads api is used for sequential read. In such case, in current code we will continue using the same InputStream to read from remote across multiple invocations of read call but with the change to let Chains open and close stream themselves we would open new Stream for every read.

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

1 participant