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

File handles are not closed for backend pwiz #48

Closed
jorainer opened this issue Aug 25, 2016 · 12 comments
Closed

File handles are not closed for backend pwiz #48

jorainer opened this issue Aug 25, 2016 · 12 comments
Assignees

Comments

@jorainer
Copy link
Collaborator

Apparently, the file handles are not closed for backend = "pwiz":

library(mzR)
library(msdata)
f <- msdata::proteomics(full.names = TRUE, pattern = "TMT_Erwinia")
for (i in 1:1000) {
    msd <- mzR::openMSfile(f, backend = "pwiz")
    mzR::close(msd)
}

Throws the error:

Error: failed opening file: Too many open files: unspecified iostream_category error
@lgatto
Copy link
Collaborator

lgatto commented Aug 25, 2016

@thirdwing - could you check this, please.

@thirdwing thirdwing self-assigned this Aug 25, 2016
@thirdwing
Copy link

I think I know how to fix this, but I am really busy this week.

On 08/25/2016 04:43 AM, Laurent Gatto wrote:

@thirdwing https://github.com/thirdwing - could you check this, please.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#48 (comment), or
mute the thread
https://github.com/notifications/unsubscribe-auth/ABebVR8NlvxPe-MBLSt_YUeeGKPeWF3tks5qjVWugaJpZM4JsxCv.

@lgatto
Copy link
Collaborator

lgatto commented Aug 25, 2016

If you could share your thoughts, I could try to have a look. Otherwise, next week is no problem at all. Thanks.

jorainer added a commit to jorainer/mzR that referenced this issue Aug 25, 2016
@jorainer
Copy link
Collaborator Author

jorainer commented Aug 25, 2016

@thirdwing @lgatto : I think I fixed it! I've implemented the close method for mzRpwiz objects (that did just return TRUE without doing anything). In the C++ code I implemented the close method to release all resources; there does not seem to be an explicit close function to be implemented in the pwiz code, so I think that's all what we can do. @thirdwing can you please check if that's similar to what you had in mind?
It's in pull request #49

With this I don't get the error message with too many open connections.

@lgatto
Copy link
Collaborator

lgatto commented Aug 25, 2016

I see you implemented the same as for the Ramp backend. Looks good to me. @thirdwing, any comment?

@thirdwing
Copy link

The problem is in
https://github.com/sneumann/mzR/blob/master/R/methods-mzRpwiz.R#L142

I don't know why I just put a placeholder for pwiz. We need to
explicitly call destructor or a close() method like
https://github.com/sneumann/mzR/blob/master/R/methods-mzRramp.R#L66

On 08/25/2016 02:43 PM, Laurent Gatto wrote:

I see you implemented the same as for the |Ramp| backend. Looks good
to me. @thirdwing https://github.com/thirdwing, any comment?


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#48 (comment), or
mute the thread
https://github.com/notifications/unsubscribe-auth/ABebVQ0oWubocsJT0UjNk5PL3L7IsqD8ks5qjeJqgaJpZM4JsxCv.

@lgatto
Copy link
Collaborator

lgatto commented Aug 25, 2016

@thirdwing - and that's exactly what @jotsetung has done in #49, as far as I can see.

@thirdwing
Copy link

@jotsetung Sorry I missed your email.

@lgatto I think it is just what I thought.

@lgatto
Copy link
Collaborator

lgatto commented Aug 25, 2016

Great, thanks.

lgatto pushed a commit that referenced this issue Aug 25, 2016
@lgatto lgatto closed this as completed Aug 25, 2016
@jorainer
Copy link
Collaborator Author

sorry to post to the closed issue... I was just wondering who will then push the changes also to the Bioconductor svn (and when).

@lgatto
Copy link
Collaborator

lgatto commented Aug 26, 2016

I will, today.

@lgatto
Copy link
Collaborator

lgatto commented Aug 26, 2016

Done

------------------------------------------------------------------------
r120458 | l.gatto | 2016-08-26 08:40:19 +0100 (Fri, 26 Aug 2016) | 1 line

upgrade pwiz and close connection for pwiz backend

lgatto pushed a commit that referenced this issue Sep 22, 2016
* master: (340 commits)
  sort out news before committing to bioc
  Fix issue #48
  Add some documentation and link to travis CI
  add sudo to travis CI config
  add build dependency to travis CI config
  Adding travis for master branch
  make sure we are using the latest version of pwiz
  update version number
  resolve conflicts in Makevars.win
  pass on Linux
  update description
  Recompile libpwiz.a on windows
  remove std::cout/std::cerr
  cleanup
  compiled
  checkout latest pwiz
  revert added compiler switch, and fixed boost code instead to get rid of warnings
  forgot to update windows file in #33
  bump version and add KK's new pwiz.version to NEWS
  pwiz version
  ...
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

3 participants