snowflakedb / snowflake-connector-python

Snowflake Connector for Python
https://pypi.python.org/pypi/snowflake-connector-python/
Apache License 2.0
601 stars 473 forks source link

[SNOW-1669128]: Ensure SnowflakeConnection atexit function is registered on import #2061

Closed sfc-gh-rdurrani closed 1 month ago

sfc-gh-rdurrani commented 2 months ago

Please answer these questions before submitting your pull requests. Thanks!

  1. What GitHub issue is this PR addressing? Make sure that there is an accompanying issue to your PR.

    Fixes #SNOW-1669128

  2. Fill out the following pre-review checklist:

    • [ ] I am adding a new automated test(s) to verify correctness of my new code
    • [ ] I am adding new logging messages
    • [ ] I am adding a new telemetry message
    • [ ] I am modifying authorization mechanisms
    • [ ] I am adding new credentials
    • [ ] I am modifying OCSP code
    • [ ] I am adding a new dependency
  3. Please describe how your code solves the related issue.

Previously, the close_at_exit function for the SnowflakeConnection object was registered with the atexit module during initialization of a SnowflakeConnection object. This caused the Snowpark Session object's atexit function to be registered before the SnowflakeConnection's object when the Session was made without an existing SnowflakeConnection object, because the SnowflakeConnection object's atexit was only registered after it was initialized whereas the Snowpark Session object's atexit was registered when the file was imported. During the atexit for the Snowpark Session, it attempts to send telemetry regarding cleanup operations, but since it's connection is already closed, it fails to send the telemetry. Moving the registration of the atexit function here means that it will happen when the SnowflakeConnection object is imported - and so the Snowpark Session's atexit function will be registered after, and the order will be correct.

  1. (Optional) PR for stored-proc connector:
sfc-gh-rdurrani commented 2 months ago

Do folks think that this should be ported to SP? I checked the connection.py file there, and there doesn't seem to be an import of atexit, so I'm thinking it shouldn't be ported, but wanted to double check!

sfc-gh-jkew commented 2 months ago

I would be hesitant to change the atexit behavior here, as opposed to simply removing the warning to fix a failed telemetry send. This may actually be the root cause; but I also think it's a more dangerous change considering the initial problem ( a warning ).

I might be convinced otherwise though - maybe I'm just being paranoid.

sfc-gh-rdurrani commented 1 month ago

I would be hesitant to change the atexit behavior here, as opposed to simply removing the warning to fix a failed telemetry send. This may actually be the root cause; but I also think it's a more dangerous change considering the initial problem ( a warning ).

I might be convinced otherwise though - maybe I'm just being paranoid.

I totally get where you're coming from (and am open to removing the warning as well), but like you point out, I think that this is the root problem, and further, that the current behavior is incorrect. Since the Snowpark session object has a dependency on the SnowflakeConnection object, it makes sense that the snowflake connection object shouldn't be garbage collected before the Session object. I'd argue that while this change is broader, it moves us closer to a more correct garbage collection.

sfc-gh-rdurrani commented 1 month ago

Seems that the issue this was trying to resolve has been fixed by another PR!