Skip to content
This repository has been archived by the owner on Jun 1, 2023. It is now read-only.

Do not use File.each_line iterator to ensure Files are closed #116

Merged
merged 1 commit into from
May 10, 2018

Conversation

bmulvihill
Copy link
Contributor

Currently when running the specs on my Mac I get a Too many files open error. This is because when loading the DependencyGraph we are using the File.each_line iterator method, which does not close the file. This changes the DependencyGraph to use the File.each_line method which yields to a block, and properly closes the File.

@bmulvihill bmulvihill added the bug label May 9, 2018
@@ -122,16 +122,17 @@ module Scry::Completion::DependencyGraph

def parse_requires(file_path)
file_dir = File.dirname(file_path)
File.each_line(file_path).reduce([] of String) do |acc, line|
paths = [] of String
File.each_line(file_path) do |line|
require_statement = /^\s*require\s*\"(?<file>.*)\"\s*$/.match(line)
unless require_statement.nil?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: can be written as if require_statement
But that's not related to this pr

@bew
Copy link
Contributor

bew commented May 9, 2018

I don't get it, why this change fixes the file open issue? It basically doesn't change anything, it's just written differently

@bmulvihill
Copy link
Contributor Author

bmulvihill commented May 9, 2018

@bew File.each_line without a block (which is what we are using currently) will not close the File since it delegates to the iterator method here: https://github.com/crystal-lang/crystal/blob/master/src/file.cr#L628, which in turn calls File.open without a block, which simply creates a new File. Using the block, as this PR does will delegate to https://github.com/crystal-lang/crystal/blob/master/src/file.cr#L574-L581 which ensures the file is closed

@bew
Copy link
Contributor

bew commented May 9, 2018

Nice find! Can you open an issue on Crystal on this, I think this is a bug, or it should be documented

@faustinoaq
Copy link
Member

@bmulvihill Nice, Thank you! 👍

Copy link
Member

@faustinoaq faustinoaq left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Travis on macOS is passing now 🎉

@faustinoaq faustinoaq requested review from laginha87 and bew May 9, 2018 21:03
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants