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

Guard against StackoverflowErrors: prevent packages from extending FileIO.load or FileIO.save #154

Closed
wants to merge 1 commit into from

Conversation

timholy
Copy link
Member

@timholy timholy commented Sep 20, 2017

Follows up on #141 (comment)

@timholy
Copy link
Member Author

timholy commented Sep 20, 2017

CC @simonster

@timholy timholy changed the title Guard against StackoverflowErrors if packages extend FileIO.load Guard against StackoverflowErrors: prevent packages from extend FileIO.load or FileIO.save Sep 21, 2017
@timholy timholy changed the title Guard against StackoverflowErrors: prevent packages from extend FileIO.load or FileIO.save Guard against StackoverflowErrors: prevent packages from extending FileIO.load or FileIO.save Sep 21, 2017
@timholy
Copy link
Member Author

timholy commented Sep 21, 2017

I should point out that this is a dangerous PR that I won't merge unless there's substantial buy-in from other folks. It will break JLD (and maybe many other packages) until they stop extending load and save:

julia> save("myfile.jld", Dict("x"=>5))
Error encountered while saving "/tmp/myfile.jld".
Fatal error:
ERROR: JLD writer error: module should not extend FileIO.save
  Will try next writer.

Though through the magic of dispatch, it works if you just try a second time:

julia> save("myfile.jld", Dict("x"=>5))

@ssfrr
Copy link
Contributor

ssfrr commented Oct 11, 2017

I think that if the FileIO API is that you shouldn't extend load and save but instead define your own, it seems reasonable to enforce that (especially if not doing so causes stack overflows).

If the affected package authors get a heads up they should be able to fix things on their side without breaking anything, before this gets merged. Another option could be to make it a warning at first, so there's a sort of deprecation period.

It would probably be good to include a specific note in the Implementing Loaders/Savers section of the README that says that your package should define load and save methods (but not export them).

@timholy
Copy link
Member Author

timholy commented Nov 27, 2017

Thanks @ssfrr. See #164. If we decide to go ahead with this PR, before merging I propose that we send something out to discourse and give folks a couple of weeks to adjust.

For the record:

julia> Pkg.dependents("FileIO")
50-element Array{AbstractString,1}:
 "Luxor"                 
 "Kpax3"                 
 "Images"                
 "ELF"                   
 "OMETIFF"               
 "MFCC"                  
 "GLAbstraction"         
 "NRRD"                  
 "LibSndFile"            
 "RData"                 
 "AndorSIF"              
 "JLD"                   
 "LasIO"                 
 "GLBooks"               
 "PkgBenchmark"          
 "Weber"                 
 "Meshes"                
 "WAV"                   
 "StatFiles"             
 "GLVisualize"           
 "GLWindow"              
 "RigidBodyTreeInspector"
 "TestImages"            
 "MeshIO"                
 "BedgraphFiles"         
 "Ogg"                   
 "VideoIO"               
 "TensorFlow"            
 "ProfileView"           
 "FeatherFiles"          
 "MP3"                   
 "JLD2"                  
 "CSVFiles"              
 "ColorSchemes"          
 "Opus"                  
 "SwiftObjectStores"     
 "ReferenceTests"        
 "QuartzImageIO"         
 "MetaImageFormat"       
 "FLAC"                  
 "ImageView"             
 "ImageMagick"           
 "ExcelFiles"            
 "ImagineFormat"         
 "Netpbm"                
 "Augmentor"             
 "Caesar"                
 "GslibIO"               
 "EMIRT"                 
 "RDatasets"             

We could download all of these and see which ones seem to be extending FileIO's methods. This won't catch private repos, but would be better than nothing. I feel guilty that we let this go so long without documenting it unambiguously.

@rapus95
Copy link

rapus95 commented Oct 8, 2019

Stumbled over this, looked into the changes, found the ancient 2017 syntax. Happy to have newer better syntax.
Anyway, are there similar plans or was it just forgotton to be closed?

@timholy
Copy link
Member Author

timholy commented Mar 3, 2021

#295 implemented a warning

@timholy timholy closed this Mar 3, 2021
@timholy timholy deleted the teh/stackoverflow branch March 3, 2021 16:47
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

Successfully merging this pull request may close these issues.

3 participants