{"action":"created","issue":{"url":"https://api.github.com/repos/hypothesis/h/issues/5337","repository_url":"https://api.github.com/repos/hypothesis/h","labels_url":"https://api.github.com/repos/hypothesis/h/issues/5337/labels{/name}","comments_url":"https://api.github.com/repos/hypothesis/h/issues/5337/comments","events_url":"https://api.github.com/repos/hypothesis/h/issues/5337/events","html_url":"https://github.com/hypothesis/h/pull/5337","id":366436740,"node_id":"MDExOlB1bGxSZXF1ZXN0MjIwMTEwODg3","number":5337,"title":"Isolate event subscriber failures when calling `request.notify_after_commit`","user":{"login":"robertknight","id":2458,"node_id":"MDQ6VXNlcjI0NTg=","avatar_url":"https://avatars1.githubusercontent.com/u/2458?v=4","gravatar_id":"","url":"https://api.github.com/users/robertknight","html_url":"https://github.com/robertknight","followers_url":"https://api.github.com/users/robertknight/followers","following_url":"https://api.github.com/users/robertknight/following{/other_user}","gists_url":"https://api.github.com/users/robertknight/gists{/gist_id}","starred_url":"https://api.github.com/users/robertknight/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/robertknight/subscriptions","organizations_url":"https://api.github.com/users/robertknight/orgs","repos_url":"https://api.github.com/users/robertknight/repos","events_url":"https://api.github.com/users/robertknight/events{/privacy}","received_events_url":"https://api.github.com/users/robertknight/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"assignees":[],"milestone":null,"comments":2,"created_at":"2018-10-03T17:01:40Z","updated_at":"2018-10-09T05:55:40Z","closed_at":null,"author_association":"CONTRIBUTOR","pull_request":{"url":"https://api.github.com/repos/hypothesis/h/pulls/5337","html_url":"https://github.com/hypothesis/h/pull/5337","diff_url":"https://github.com/hypothesis/h/pull/5337.diff","patch_url":"https://github.com/hypothesis/h/pull/5337.patch"},"body":"When using `request.notify_after_commit` to trigger an event after the current web request's transaction is committed, an exception in one event subscriber can cause other subscribers not to run. The order in which subscribers are run is not specified. This makes it hard to reason about the eventual state if any one subscriber fails to run.\r\n\r\nThis relates to a [Sentry issue](https://github.com/hypothesis/h/issues/5311) I'm looking into where failures sending a message to the message broker in order to trigger a websocket notification can also cause other, more important tasks to fail. These tasks include sending reply notification emails and indexing the annotation for search. Fixing the actual cause of the write failure itself is something I also plan to look into, but I think it would be good to reduce the \"blast radius\" of the problem.\r\n    \r\nThis commit replaces `request.registry.notify` with equivalent code using lower-level zope functions, which wraps each subscriber in a catch block. Therefore one subscriber failure won't affect others.\r\n\r\nI also added some basic documentation to the `EventQueue` class to explain what it is for.\r\n\r\n_This is a partial mitigation for https://github.com/hypothesis/h/issues/5311_."},"comment":{"url":"https://api.github.com/repos/hypothesis/h/issues/comments/428069520","html_url":"https://github.com/hypothesis/h/pull/5337#issuecomment-428069520","issue_url":"https://api.github.com/repos/hypothesis/h/issues/5337","id":428069520,"node_id":"MDEyOklzc3VlQ29tbWVudDQyODA2OTUyMA==","user":{"login":"robertknight","id":2458,"node_id":"MDQ6VXNlcjI0NTg=","avatar_url":"https://avatars1.githubusercontent.com/u/2458?v=4","gravatar_id":"","url":"https://api.github.com/users/robertknight","html_url":"https://github.com/robertknight","followers_url":"https://api.github.com/users/robertknight/followers","following_url":"https://api.github.com/users/robertknight/following{/other_user}","gists_url":"https://api.github.com/users/robertknight/gists{/gist_id}","starred_url":"https://api.github.com/users/robertknight/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/robertknight/subscriptions","organizations_url":"https://api.github.com/users/robertknight/orgs","repos_url":"https://api.github.com/users/robertknight/repos","events_url":"https://api.github.com/users/robertknight/events{/privacy}","received_events_url":"https://api.github.com/users/robertknight/received_events","type":"User","site_admin":false},"created_at":"2018-10-09T05:55:40Z","updated_at":"2018-10-09T05:55:40Z","author_association":"CONTRIBUTOR","body":"> I have some concerns that looping like this may impact performance, maybe it loops underneath and it's not a big deal-I haven't looked. \r\n\r\nThe previous method (`request.registry.notify`) is indeed [just a loop](https://docs.pylonsproject.org/projects/pyramid/en/latest/_modules/pyramid/registry.html#Registry.notify) internally. Additionally there are only a small number of subscribers (three or four). I'm not worried about a performance regression here.\r\n\r\n> Have you considered wrapping each subscriber as they get added in add_subscriber with some code that implements this try-except-reporting behavior or something similar rather than what you've done here?\r\n\r\nYes, I did consider this approach. However I saw two problems:\r\n\r\n1. Events are dispatched in different contexts. In this code, we are dispatching events at the end of a request after the database transaction has been committed, so we definitely wouldn't want to rollback if there is a failure. Elsewhere in the codebase we call `request.registry.notify` while a transaction is active. In that context we probably do want to keep the current behaviour of aborting the operation and rolling back the transaction if there is a failure.\r\n\r\n2. Someone could add a new event handler in future and forget to wrap the subscriber.\r\n\r\n> The main change is I submitted a commit to fixup the tests cause it was easier than putting it all in comments-please take a look. I found the tests a little confusing and difficult to follow. \r\n\r\nLet's sync up about this later when you're online and go through your changes in a bit more detail. One thing I do want to say is that in this PR I only added or changed tests that are relevant to the functional change I'm making. I'm totally open to doing some refactoring of existing code if that will make it easier to follow, but I suggest that should happen in a separate PR to avoid confusing other developers in future."}}
{"actor":{"display_login":"robertknight"}}