Skip to content

fix: Add a optional grace time period to shutdown method #40

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 2 commits into from
Closed

fix: Add a optional grace time period to shutdown method #40

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
954546a
Add a optional grace time period to shutdown method
bohan-amplitude Nov 15, 2022
ff14932
Update test_client.py
bohan-amplitude Nov 15, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions src/amplitude/client.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
Classes:
Amplitude: the Amplitude client class
"""

import time
from typing import Optional, Union, List

from amplitude.config import Config
Expand Down Expand Up @@ -163,7 +163,9 @@ def remove(self, plugin: Plugin):
self.__timeline.remove(plugin)
return self

def shutdown(self):
def shutdown(self, grace_time_milliseconds=0):
"""Shutdown the client instance, not accepting new events, flush all events in buffer"""
self.configuration.opt_out = True
self.__timeline.shutdown()
if grace_time_milliseconds > 0:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the quick update! Not sure whether we should add this here, as if shutdown is exposed as public API, user can do the time.sleep if they need. Do we consider to use async tasks for the worker? https://docs.python.org/3/library/asyncio-task.html.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The current worker use threading. Switch to coroutine may need some work to re-design and implement the retry logic. shutdown() is a public interface suppose to be called before application exit. I have run some tests. Add ether flush or shutdown after tracking will stop the error of can not schedule future. Let me ask customer if they added these call.

from amplitude import *


client = Amplitude('')

client.track(BaseEvent(
    event_type='Python Event',
    user_id='[email protected]',
    ip='127.0.0.1',
    event_properties={
        'keyString': 'valueString',
        'keyInt': 11,
        'keyBool': True
    }
))

# client.flush()
client.shutdown()```

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see, I thought this fix is for the other issue (timing issue). I cannot reproduce the error can not schedule future in both Python 3.9 and 3.10. If adding this can fix the issue, I am good with change. Just to double check, will this be stable with the usage of sleep?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bohan-amplitude Can you please first enable py37-p311 pipelines, so we can see what really happens with different python versions, running only on py36 is insane, that version pinned for the fjords long time ago.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @ssbarnea, just shared open another pull request with workaround for this issue at #43, feel free to take a look and let us know your thoughts. Seems that passes the test on py3.6 to py3.11.

time.sleep(grace_time_milliseconds / 1000)
2 changes: 1 addition & 1 deletion src/test/test_client.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ def setUp(self) -> None:
configuration=Config(flush_queue_size=10, flush_interval_millis=500))

def tearDown(self) -> None:
self.client.shutdown()
self.client.shutdown(100)

def test_amplitude_client_track_success(self):
post_method = MagicMock()
Expand Down