Review comments - Part 1 #70
Conversation
Codecov Report
@@ Coverage Diff @@
## main #70 +/- ##
==========================================
+ Coverage 90.82% 90.95% +0.13%
==========================================
Files 7 7
Lines 828 829 +1
==========================================
+ Hits 752 754 +2
+ Misses 76 75 -1
Continue to review full report at Codecov.
|
src/ansys/openapi/common/_util.py
Outdated
| @@ -233,6 +233,9 @@ def __init__(self): | |||
| async def get_auth_code(self): | |||
| return self._auth_code.get(block=True) | |||
|
|
|||
| def __del__(self): | |||
There was a problem hiding this comment.
So even though the OIDCCallbackHTTPServer object was deleted, the server wasn't necessarily closed? Makes sense I suppose.
There was a problem hiding this comment.
I think it's specifically the socket binding, the garbage collector was probably cleaning up the sever in time, but I think it wasn't actively releasing the socket binding. This looks like it fixes the issue I was seeing, but I'm not confident. I'll address the other comments here and see if the issue recurs in CI runs.
| @@ -200,7 +200,6 @@ async def await_callback(): | |||
| if _log_tokens: | |||
| logger.debug(f"[TECHDOCS]Received authorization code: {auth_code}") | |||
| self._callback_server.shutdown() | |||
| del self._callback_server | |||
There was a problem hiding this comment.
Have you confirmed that implementing the __del__() method below is sufficient to completely shutdown the server without this explicit del? It might be good to leave this in place, or maybe call server_close? I'm just conscious of accidentally leaving a server socket open.
There was a problem hiding this comment.
It might be good to leave this in place, or maybe call server_close? I'm just conscious of accidentally leaving a server socket open.
We've had issues with pymapdl where we forget to collect the MAPDL instance and leave the remote/local process running. Checking collection in unit testing is helpful.
There was a problem hiding this comment.
Calling del here was not a practical solution in the long term. Calling shutdown should stop the server, and the __del__() method does seem to be sufficient to release the socket binding.
There was a problem hiding this comment.
I'm still a bit paranoid about this. Is it possible to add a test that checks that once the OIDC callback server is deallocated the port binding is gone. Is it possible to write a unit test that asserts the port really is open?
There was a problem hiding this comment.
I imagine we could spin a callback server up, hit it with a request, shut it down then see if we bind to the port. I suspect we will be slightly at the mercy of the Linux network stack, but I can give it a go and see.
Testing it in place is on the list of things to do, but it's nontrivial, we need to mock a web browser and IDP (probably adapted from how requests_oauthlib or httpx_auth do it).
pyproject.toml
Outdated
| build-backend = "flit_core.buildapi" | ||
|
|
||
| [project] | ||
| name = "ansys_openapi_common" |
There was a problem hiding this comment.
Is this the name that will appear on pypi? If so, I think it should be 'ansys-openapi-common'.
This PR switches the build backend and consolidates requirements files into pyproject.toml.
Closes #60 - Streamline requirements
Closes #61 - Drop 3.6
Closes #62 - Rename CI/CD step