fix: stub.serve() shutdown #108
Conversation
|
This doesn't seem 100% obvious to me – what if someone wants to call |
|
I think that's a weird case. The |
|
Would prefer to solve it in a nicer way – fine to merge this but let's at least add some comments explaining what's the purpose of this code. Would also prefer to add a test. |
modal/client.py
Outdated
| @@ -137,7 +139,16 @@ async def _start(self): | |||
|
|
|||
| logger.debug("Client: Done starting") | |||
|
|
|||
| async def set_pre_stop(self, pre_stop: Callable[[], None]): | |||
There was a problem hiding this comment.
This wouldn't have to be an async method right?
|
Agree that this is not a nice solution, but prefer to defer the biggest refactor until after webhook updates are shipped. Will add the comments + test though 👍 |
|
If it's hard to add the test, it's not the end of the world to skip it. Would be great to add some comments though! |
thundergolfer
force-pushed
the
jonathon/fix-stub-serve-shutdown-dec1
branch
from
f2a48f2
to
942efa6
Compare
thundergolfer
force-pushed
the
jonathon/fix-stub-serve-shutdown-dec1
branch
from
867bd4e
to
60207ca
Compare
client_test/stub_test.py
Outdated
| stub = Stub() | ||
| fake_disconnect = AsyncMock() | ||
| with modal.client.Client(servicer.remote_addr, api_pb2.CLIENT_TYPE_CLIENT, ("foo-id", "foo-secret")) as client: | ||
| with patch.object(modal.app._App, "disconnect", fake_disconnect): |
There was a problem hiding this comment.
Awesome with test!
Instead of mocking the app, you could probably just see if the AppClientDisconnect call was called on the servicer fixture
There was a problem hiding this comment.
Cheers, I'll try that 👍
There was a problem hiding this comment.
Hmm, seems doing that has triggered the need for @skip_in_github
There was a problem hiding this comment.
A mock-y test that runs in CI is probably better than a non-mock test that doesn't run in CI, so I'll go back to that.
There was a problem hiding this comment.
Damn, the mock is flaky too.
This issue would be solved by solving MOD-533.
thundergolfer
force-pushed
the
jonathon/fix-stub-serve-shutdown-dec1
branch
from
fcbf901
to
cd708fc
Compare
|
Attempt at removing this here: #1523 |
stub.serve()is currently unable toawait app.disconnect()and stop an app, becauseasync_utils.on_shutdown(AioClient.stop_env_client())runs before the disconnect call and disconnects theclient. (it's a race that thedisconnect()never wins)I tried a couple dozen things, but can't get around the fact that once Ctrl + C has initiated the async loop's shutdown, you can't control the ordering of the app disconnection and the client stop.
The proper fix is probably to rewrite all
clientusage to be done by context manager, but that's much more involved than this stop-gap I've done here.Ref: Shutdown hook is from https://github.com/modal-labs/modal/pull/2957.