wjakob / nanobind

nanobind: tiny and efficient C++/Python bindings
BSD 3-Clause "New" or "Revised" License
2.29k stars 191 forks source link

Fix several sources of undefined behavior in type casters #549

Closed oremanj closed 4 months ago

oremanj commented 5 months ago
oremanj commented 5 months ago

I'm not sure yet what's going on with the failing tests - all of them are because the destruction of one of the Example objects is being delayed past the end of the test. It happens on different builds each time I run, but consistently 2-3 of them. Haven't been able to reproduce it locally yet. Adding an extra collect() didn't help.

wjakob commented 4 months ago

Hi @oremanj, just to keep you posted: I'm currently swamped with a deadline (in ~2 weeks), and then I'm moving to another country. I won't have time to debug the test issue in a near timeframe.

wjakob commented 4 months ago

I looked into the GC issue and was eventually able to reproduce it on a windows machine. As far as flakes go, it seems relatively benign -- some instances not being collected by the time they're supposed to, but then they do get deleted later on.

It's weird, as if something was wrong with the GC. Or somebody else is holding further references. Perhaps it's a combination of the GC and the extra rewriting that Pytest does to capture exceptions and warnings with extra contexts to provide readable output. But then it's weird that this only seems to happen on Windows.

The issue goes away simply by refactoring the big test into smaller ones. So I will leave it at this for now.

wjakob commented 4 months ago

This PR was merged out of band in commit c36584e577686500842e0cf1ac04f45608bd73de. (I officially merged the PR but will force-push the tweaked version. Just in case it plays a role for the bean counting somewhere ;))

Thanks for your work on this, it's a great improvement.