Skip to content
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.

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

Async client added #62

Merged
merged 3 commits into from
Apr 25, 2011
Merged

Async client added #62

merged 3 commits into from
Apr 25, 2011

Conversation

buriy
Copy link
Contributor

@buriy buriy commented Dec 23, 2010

=)

@dcramer
Copy link
Member

dcramer commented Dec 23, 2010

This looks great! Does this work at all in synchronous servers like Apache?

@buriy
Copy link
Contributor Author

buriy commented Dec 23, 2010

Yes, it does work properly, the only problem is that these commits don't include a way to terminate message sending thread when the main thread was terminated.
I plan to add something like atexit check + 3 seconds delay.

@buriy
Copy link
Contributor Author

buriy commented Dec 23, 2010

Wait a minute, you wanted to ask about not-reentrant client code like CGI? Then, it has a chance to exit before sending a single error message. But I use sys.excepthook + create_from_exception for that case.

@buriy
Copy link
Contributor Author

buriy commented Dec 23, 2010

Ah, you must be speaking about Apache mpm_prefork... I'm using it on lighttpd+FastCGI and custom API servers only now.
Again, then you need to ensure client won't be stopped before sending the message, or killed after some timeout. I.e. if sentry server shows 500 error and linger each connection for a minute, you don't want to kill your web servers speed and would like to set 5 seconds timeout after main thread was terminated.
Any ideas how to ensure proper timeouts for this case? Would atexit+timeout be enough?

@dcramer
Copy link
Member

dcramer commented Dec 23, 2010

I'm actually not too familiar w/ hanging threads for such purposes which is why I was curious :)

dcramer added a commit that referenced this pull request Apr 25, 2011
@dcramer dcramer merged commit cf78ac1 into getsentry:master Apr 25, 2011
@tmm1
Copy link

tmm1 commented Apr 25, 2011

Hey there,

Soon after the new GitHub Merge Button feature was released today we discovered a bug in the merge procedure in certain cases. This pull request was affected by the bug, causing code to not actually get merged even though it looks from the history as though it was.

To bring the master branch back to its expected state, commit this diff of changes that were missed by the merge:

diff --git a/sentry/client/base.py b/sentry/client/base.py
index 6663561..5789271 100644
--- a/sentry/client/base.py
+++ b/sentry/client/base.py
@@ -127,6 +127,9 @@ class SentryClient(object):

         return message_id

+    def send_remote(self, url=None, data=None):
+        return urlread(url, post=data, timeout=conf.REMOTE_TIMEOUT)
+
     def send(self, **kwargs):
         "Sends the message to the server."
         if conf.REMOTE_URL:
@@ -137,7 +140,7 @@ class SentryClient(object):
                     'key': conf.KEY,
                 }
                 try:
-                    urlread(url, post=data, timeout=conf.REMOTE_TIMEOUT)
+                    self.send_remote(url=url, data=data)
                 except urllib2.HTTPError, e:
                     body = e.read()
                     logger.error('Unable to reach Sentry log server: %s (url: %%s, body: %%s)' % (e,), url, body,

Sorry for the inconvenience. If you have any questions or need help, do not hesitate to contact support.

@github-actions github-actions bot locked and limited conversation to collaborators Dec 24, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants