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

RqMultipart.Base IOException with very large TXT files. #626

Closed
wild-lotus opened this issue Feb 27, 2016 · 42 comments
Closed

RqMultipart.Base IOException with very large TXT files. #626

wild-lotus opened this issue Feb 27, 2016 · 42 comments

Comments

@wild-lotus
Copy link

When dealing with requests with very large TXT files like this one I get this stack trace (using takes 0.31.3):

java.io.IOException: Too many open files in system
    at java.io.UnixFileSystem.createFileExclusively(Native Method)
    at java.io.File.createTempFile(File.java:2024)
    at java.io.File.createTempFile(File.java:2070)
    at org.takes.rq.RqMultipart$Base.make(RqMultipart.java:256)
    at org.takes.rq.RqMultipart$Base.requests(RqMultipart.java:239)
    at org.takes.rq.RqMultipart$Base.<init>(RqMultipart.java:151)

There are at least a couple of issues that seeam to be similar: #254 #512.

@davvd
Copy link

davvd commented Mar 1, 2016

@yegor256 please do something about this issue

@yegor256
Copy link
Owner

yegor256 commented Mar 2, 2016

@CarlosGines can you please show the code, which will allow us to reproduce this?

@wild-lotus
Copy link
Author

@yegor256 sure, you can reproduce it with this class:

public final class App {
  public static void main(final String... args) throws Exception {
    new FtBasic(
        new Take() {
            @Override
            public Response act(final Request req) {  
                try {
                    final Request file = new RqMultipart.Base(req);
                } catch (Exception e) {
                    e.printStackTrace();
                }
                return new RsText("Hello, world!");
            }
        },
        8080
    ).start(Exit.NEVER);
  }
}

And this html:

<!DOCTYPE html>
<html>
<head>
</head>
<body>
    <form method="post" id="upload" action="http://localhost:8080" enctype="multipart/form-data">
        <fieldset>
            <input id="file-name" name="name" autocomplete="off" placeholder="attachment name..." size="30" maxlength="50">
            <input id="file-binary" name="file" type="file">
            <label for="file-submit"></label>
            <input id="file-submit" type="submit" value="Upload">
        </fieldset>
    </form>
</body>
</html>

@yegor256
Copy link
Owner

yegor256 commented Mar 2, 2016

@CarlosGines and then I have to upload a huge file?

@original-brownbear
Copy link
Contributor

@CarlosGines is this even still an issue you can reproduce now that #634 is merged? In my opinion that PR should resolve this situation, since it fixes file descriptors not being released in the RqMultipart.Base.

@wild-lotus
Copy link
Author

@original-brownbear yes, still there!

@wild-lotus
Copy link
Author

@yegor256 that's right. I provided a huge file to test it, this one.

@yegor256
Copy link
Owner

yegor256 commented Mar 5, 2016

@original-brownbear do you think it's a bug in Takes?

@original-brownbear
Copy link
Contributor

@yegor256 there certainly was a bug that would show exactly like this, but it should have been resolved by #634, also it should not be reproducible this trivially ( see below ).
The explanation in #634 eventually turned a little long, so to summarize:

  • org.takes.rq.RqMultipart$Base opened temp file streames and wrapped them in CapInputStream
  • CapInputStream#close was not correctly implemented causing those to never actually close
  • eventually you run out of file descriptors

That being said, if this error is happening in 100% of the cases, even after a clean reboot I can't see how this is Takes fault. Even if it was leaking file descriptors like hell, creating one more temp file right after starting clean should always be possible.

@original-brownbear
Copy link
Contributor

@CarlosGines please see above comment, can you reproduce this in 100% of cases on a clean reboot even?

@wild-lotus
Copy link
Author

@original-brownbear yes, I still reproduce it. You can try it too using the code and the file I provide above.

@wild-lotus
Copy link
Author

@original-brownbear when you say "clean reboot", is there something special I need to do apart from normal rebooting? Some files I need to delete or something?

@original-brownbear
Copy link
Contributor

@CarlosGines nope just restarting your computer :) nothing special.
Two things though :)

  1. You have to convince @yegor256 here, not me. He's the architect in this project, while I'm not even part of the team at this time. I merely offered an explanation as to where this could be coming from. Given that you can reproduce this over and over it appears that explanation is not correct here.
  2. Pasting a code snippet like the above is a very bad way of reproducing the issue. If you think about it, It'll probably take in excess of 5 minutes to set this code up, run it and then report back here. I don't think anyone is going to do that really. I suggest simply turning this into an actual test case and linking a branch here. This would clearly prove there is an issue and already add value in form of the test :) ... and again don't prove it to me, prove it to the architect here :)

@wild-lotus
Copy link
Author

@original-brownbear thank you very much, I will take it into account!

@wild-lotus
Copy link
Author

@yegor256 is this info already enough to raise the bug?
It is there for sure. The while in RqMultipart.Base.request() loops forever, because of RqMultipart.Base.buffer and the while loop in RqMultipart.Base.copy().
I have been trying to catch it in a test, but it is taking me far more than even 30 min.

@yegor256
Copy link
Owner

yegor256 commented Mar 9, 2016

@CarlosGines OK, let's call it a bug and see what we can do. thanks.

@yegor256 yegor256 added the bug label Mar 9, 2016
@davvd davvd added this to the 1.0 milestone Mar 10, 2016
@davvd
Copy link

davvd commented Mar 10, 2016

@carlosgines since there is no milestone yet I set it to "1.0"

@davvd
Copy link

davvd commented Mar 10, 2016

@carlosgines thanks a lot for reporting, 15 mins added to your acc, pmt ID AP-2BM422107K740383H

@davvd
Copy link

davvd commented Mar 10, 2016

@dalifreire please proceed, it's yours

@exper0
Copy link
Contributor

exper0 commented Mar 11, 2016

@yegor256 @original-brownbear @CarlosGines this is supposed to be fixed in scope of #576

@dalifreire
Copy link
Contributor

@davvd @exper0 @yegor256 @original-brownbear @CarlosGines Let me know if I must stop or go ahead in this issue, ok?

@wild-lotus
Copy link
Author

@dalifreire you can test it yourself whether it is fixed or not. Follow the instructions I provide in my comments above.

@dalifreire
Copy link
Contributor

@CarlosGines I think I didn't understand the problem. Could you better explain exactly what must be done?

@davvd @yegor256 @exper0 @original-brownbear 30 minutes to implement this task. I think I have not enough knowledge of 'takes' to fix that in this time. It is not enough because the problem is much bigger and requires more work than the slotted time allows.

@wild-lotus
Copy link
Author

@dalifreire First thing is reproducing it. Get the class I provide above and run as explained in Takes README.md Then create the html I provide above and upload the file I provide. You must get that exception of "too many files".

Then, before trying to solve it, try to catch it in a test. If you do that, it is probably just enough. Read here about PDD for more info. I have also mentioned above what I had found so far about the problem, something related to the while loops looping forever.

If there are even more things that you think that you need to understand before doing this, create a new issue where you explain them and ask for a solution (try to be as precise as possible). Then say here that that issue is an impediment for you to solve this task.

I hope this helps you!

@dalifreire
Copy link
Contributor

@CarlosGines Thanks for your explanation.

@davvd Please, assign someone else.

@wild-lotus
Copy link
Author

@davvd I would set #651 as an impediment for this ticket, and solve it before assigning it to anybody.

@yegor256
Copy link
Owner

@davvd assign someone else

@davvd
Copy link

davvd commented Mar 15, 2016

@davvd assign someone else

@yegor256 all right, we'll find someone else for this task

@davvd davvd removed the @dalifreire label Mar 15, 2016
@davvd
Copy link

davvd commented Mar 21, 2016

@triberraar this task is for you, keep these principles in mind, and don't hesitate to ask any technical questions... Task's budget is 30 mins (see this for explanation)

@davvd
Copy link

davvd commented Apr 6, 2016

@triberraar check this "no obligations principle".. This task is on your name for at least 15 days. If you can't close it within the next 48 hours I will have to assign someone else to it. This article should help if you're stuck. added -30 to your rating, now it is equal to -30

@davvd
Copy link

davvd commented Apr 11, 2016

@triberraar this task is taking too long, I have to change the performer, sorry. Please stop working with it right now. See our no obligations principle

-60 added to your rating, at the moment it is: -150

@davvd davvd removed the @triberraar label Apr 11, 2016
@davvd
Copy link

davvd commented Apr 11, 2016

@essobedo the task is yours, please go ahead

@essobedo
Copy link
Contributor

@davvd Here is my PR #659 please find a reviewer

@essobedo
Copy link
Contributor

@CarlosGines @yegor256 @exper0 @original-brownbear FYI the bug still exists, I've just proposed a PR to fix it

@davvd
Copy link

davvd commented Apr 12, 2016

@davvd Here is my PR #659 please find a reviewer

@essobedo OK, got it

@essobedo
Copy link
Contributor

@CarlosGines the PR has been merged, you can close the ticket, thx in advance

@wild-lotus
Copy link
Author

@essobedo thank you, #626 closed.

@essobedo
Copy link
Contributor

@davvd the ticket is closed, it is ready for the QA, thx in advance.

@davvd
Copy link

davvd commented Apr 19, 2016

@ypshenychka please, let us know what do you think about this ticket, according to our QA rules

@ypshenychka
Copy link

@davvd Quality is good here.

@davvd
Copy link

davvd commented Apr 19, 2016

@davvd Quality is good here.

@ypshenychka thank you

@davvd
Copy link

davvd commented Apr 19, 2016

@essobedo I added 10 mins to @ypshenychka (for QA review) in transaction 84077467

30 mins sent to your balance (ID 84077497), many thanks! It took 160 hours and 45 mins.

+30 added to your rating, at the moment it is: +465

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

No branches or pull requests

8 participants