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

Give some more things logcontexts #4077

Merged
merged 3 commits into from Oct 23, 2018

Conversation

2 participants
@richvdh
Member

richvdh commented Oct 20, 2018

No description provided.

@richvdh richvdh requested a review from matrix-org/synapse-core Oct 20, 2018

@erikjohnston

Also, sytests are unhappy

finally:
self._is_processing = False
def process():
self._is_processing = True

This comment has been minimized.

@erikjohnston

erikjohnston Oct 22, 2018

Member

Can we move this out of the process function please, as its not clear to me that self._is_processing will get set before we yield

This comment has been minimized.

@richvdh

richvdh Oct 22, 2018

Member

where would you have it moved to?

This comment has been minimized.

@richvdh

richvdh Oct 22, 2018

Member

oh, iswym. yes ok

This comment has been minimized.

@richvdh

richvdh Oct 22, 2018

Member

done, though I'm not entirely sure I agree it's better. In particular if run_as_background_process fails to start the process for some reason (stack overflow, perhaps), then we'll get stuck with _is_processing = True forever.

This comment has been minimized.

@erikjohnston

erikjohnston Oct 23, 2018

Member

Hmm, yes. We could move the if self._is_processing check inside the function, which would be basically the same as before except we're wrapping the entirety of the function in a run_as_background_process.

This comment has been minimized.

@richvdh

richvdh Oct 23, 2018

Member

yeah, though run_as_background_process has enough stuff in it that I'd prefer to short-circuit it if possible.

@richvdh richvdh requested a review from erikjohnston Oct 22, 2018

@erikjohnston

This comment has been minimized.

Member

erikjohnston commented Oct 23, 2018

(I commented #4077 (comment) btw)

@erikjohnston

I think this is probably fine. As per comment, we may actually want to move the if self._is_processing check into the function, but I'll leave it up to you to decide what's best there.

@richvdh richvdh merged commit b3f6ddd into develop Oct 23, 2018

6 checks passed

ci/circleci: sytestpy2merged Your tests passed on CircleCI!
Details
ci/circleci: sytestpy2postgresmerged Your tests passed on CircleCI!
Details
ci/circleci: sytestpy3merged Your tests passed on CircleCI!
Details
ci/circleci: sytestpy3postgresmerged Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@richvdh richvdh deleted the rav/more_logcontexts branch Oct 23, 2018

@richvdh richvdh added this to To Do in Backend Core Team via automation Oct 25, 2018

@richvdh richvdh moved this from To Do to Done - Operations in Backend Core Team Oct 25, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment