search

Rblp / Rblpapi

R package interfacing the Bloomberg API from https://www.bloomberglabs.com/api/
Other
167 stars 75 forks source link

Debug BDS segfault #253 #346

Closed klin333 closed 2 years ago

klin333 commented 2 years ago

Debugging and potential fix of BDS crashes from #253

Added a bds_debug function to send thousands of bds requests in rapid succession. This helps reliably and quickly reproduce the error. See also tinytests/test_segfault.R

My hunch is that unprotected SEXP in bds.cpp get destroyed by garbage collection. After changing SEXP to Rcpp::RObject throughout bds.cpp, I could no longer reproduce the error. However, I do not have sufficient theoretical knowledge of SEXP to know for sure.

eddelbuettel commented 2 years ago

Besides switching to RObject, we could also wrap Shield<> or Protect<> around the SEXP to have a more minimal change. Otherwise concur.

eddelbuettel commented 2 years ago

Could you, if you have another minute or two, maybe try replacing the SEXP foo uses and assignments with Rcpp::Shield<SEXP> foo throughout? That is done more commonly.

(RObject is / was good too. At one point it was the top of our object hierarchy but things got moved around. It is not that widely used / idiomatic Rcpp use these days. Mostly stylistic though. You put your needle on the issue.)

klin333 commented 2 years ago

Might need some assistance with Shield, getting compiler errors everywhere, to the point where the only way for me to compile is to have no Shield and be back to where we began.

Something like Rcpp::Shield<SEXP> foo; doesn't even compile with no matching function for call to 'Rcpp::Shield<SEXPREC*>::Shield()'. I have very minimal knowledge about templates to figure this out...

Also note that an earlier commit 7f3531d3 deliberately moved away from Shield.

eddelbuettel commented 2 years ago

I concur. I just gave it a spin on a checkout of your branch and that is a fight not worth fighting. Switching to RObject is a net improvement, and you proposed a fine PR. I say we go with it.

klin333 commented 2 years ago

Would it be easier if I make a new fork and just git cherry-pick that 1 commit which actually did the fix? Ie remove the bds_debug commit and remove the tinytest commit.

Eg https://github.com/klin333/Rblpapi/tree/bds_segfault

It looks like if I do that, i'll make a new pull request with the new branch which just has the single commit

eddelbuettel commented 2 years ago

Sure.

(Random idea: hard reset three commits back then just cherry pick the one commit in. No idea if that works.)

Throwing it away and doing a clean PR is easiest. There are worse things than a closed PR. (If you are redoing things please look at ChangeLog and pretty-please consider adding an entry with name and email.)

klin333 commented 2 years ago

ok, redone as new pull request with just the fix, and ChangeLog entry.

eddelbuettel commented 2 years ago

So I guess we should / can close this one.