[JAVA-462] GridFSInputFile does not close input streams when constructed by the driver. Created: 28/Oct/11 Updated: 31/Oct/11 Resolved: 31/Oct/11 |
|
| Status: | Closed |
| Project: | Java Driver |
| Component/s: | None |
| Affects Version/s: | None |
| Fix Version/s: | 2.7 |
| Type: | Bug | Priority: | Major - P3 |
| Reporter: | Ryan Nitz | Assignee: | Antoine Girbal |
| Resolution: | Done | Votes: | 0 |
| Labels: | None | ||
| Remaining Estimate: | Not Specified | ||
| Time Spent: | Not Specified | ||
| Original Estimate: | Not Specified | ||
| Description |
|
There is an issue here that the input stream is not being closed (when constructed by the driver). Since you can construct a GridFSInputFile with an InputStream, it is really the callers responsibility to close the intput stream object. It would be presumptuous for us to close that input stream. https://github.com/mongodb/mongo-java-driver/pull/50 |
| Comments |
| Comment by Ryan Nitz [ 31/Oct/11 ] | |||||||||||
|
Merged into master. Thanks for the patch! | |||||||||||
| Comment by Green Luo [ 30/Oct/11 ] | |||||||||||
|
Fair enough. The new pull request with the suggested solution sent: https://github.com/mongodb/mongo-java-driver/pull/51 | |||||||||||
| Comment by Ryan Nitz [ 28/Oct/11 ] | |||||||||||
|
If someone is modifying an input stream before calling save/operating on (or via another thread) then they are definitely going to have issues that are impossible to defend against. Under your scenario, are you proposing that the user leaves pointers to the InputStream objects open that are held in a session object? This is usually not a good idea as it will likely cause resource leaks. It might make sense to write the files uploaded immediately to a staging/temporary location and then move to the permanent location when they are committed by the user. This will allow you to easily cleanup files that have been abandoned. Your argument definitely has merit, but it is a bad practice by a driver to close streams you has not opened. Perhaps a solution that works for all is putting an optional flag in the constructor which tells the driver to close the input stream after persisting. | |||||||||||
| Comment by Green Luo [ 28/Oct/11 ] | |||||||||||
|
I see this is purely a design decision of API. the STL container by default will NOT clean up the data it points to when it is destroyed, while boost ptr_container does. It's all depend on how you communicate the contract of the API to application developers. As an application developer I 100 percent vote for GridFSInputFile to close the stream when my data is saved. As it's purely the purpose of the input stream object been created. If someone reuse the input stream object he is not doing a clean design. What if somebody close or rewind that input stream object when _readStream2Buffer method is reading data from it? You can't really prevent people from doing bad things. So stick to the design which do favor to good application developers is the correct decision to me. The coding block you showed above is just the simplest usage of resources. Here is one real case which couldn't fit into that pattern: 1. User upload a photo in the html view As you see the control flow is not able to be put into a single coding block as it's across processes. That's why I prefer that GridFSInputFile to close the inputstream. You could argue that user can construct the GridFSInputFile with the File instance, but theoretically people could pass in a input stream object, like someone might want to construct a bufferedinputstream and pass into the GridFS api. | |||||||||||
| Comment by Ryan Nitz [ 28/Oct/11 ] | |||||||||||
|
Related to the InputStream objects that the driver generates... you are absolutely right. | |||||||||||
| Comment by Ryan Nitz [ 28/Oct/11 ] | |||||||||||
|
Per my comments in the pull request: If you didn't open it, you shouldn't close it. There are use cases where people may want to perform additional operations on the input stream. It would be presumptuous to think that this is not the case.
| |||||||||||
| Comment by Green Luo [ 28/Oct/11 ] | |||||||||||
|
I think it's not an absolute rule that it's caller's responsibility to close the input stream object passed in. It's rather a good API design that indicates it's callee's responsibility to close the input stream in this case, as caller has no knowledge that when the input stream object has been used and could be dismissed. Here is the logic: 1. Caller construct an input stream, say from Amazon S3. | |||||||||||
| Comment by Ryan Nitz [ 28/Oct/11 ] | |||||||||||
|
This can be closed when the Java driver actually constructs the input. One possible solution is to make this closeable? http://download.oracle.com/javase/6/docs/api/java/io/Closeable.html |