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

Close sessions where user is anonymous but is_valid_public_url is false #151

Merged
merged 1 commit into from Mar 17, 2020

Conversation

kkoz
Copy link
Contributor

@kkoz kkoz commented Mar 13, 2020

Previously, if a request is made where is_valid_public_url is false but is_anonymous is true, a connection would be created but would never be closed. This PR closes those connections to prevent resource leaks.

Testing this PR

  1. required setup
  • Install this version of omeroweb
  • You need a public user (to make is_anonymous True)
  • Need to have omero.web.public.url_filter configured without webclient/keepalive_ping (to make is_valid_public_url False for webclient/keepalive_ping requests)
  1. actions to perform
  • Navigate to a public page (e.g. a pathviewer image) which uses a keepalive ping.
  • Open several tabs like this.
  1. expected observations
  • The thread count used by omeroweb or gunicorn should not increase over time, even as keepalive requests are made by these pages.
  • Without this PR, the number of threads in use (e.g. result of ps -Fw -u omero Hh |grep gunicorn |wc -l) will increase over time.

@chris-allan
Copy link
Member

Builds failing due to Teemu/pytest-sugar#187; see #152.

/cc @sbesson, @will-moore, @joshmoore

@chris-allan
Copy link
Member

Build now passing after merge of #152 and Travis rebuild.

@sbesson
Copy link
Member

sbesson commented Mar 16, 2020

--include

@will-moore
Copy link
Member

Code change makes total sense.
To test the fix as described, I need to set-up public user with the right url regex etc...

@manics
Copy link
Member

manics commented Mar 17, 2020

@kkoz Just out of interest, how did you find this leak? Is there anything we can do to detect these in future?

@chris-allan
Copy link
Member

@manics We found it on a customer system where this configuration was present. I can't think of any way off hand to easily detect these in the future aside from monitoring OMERO.web thread counts.

@will-moore
Copy link
Member

Instead of pathviewer I added ping every second to webgateway viewer.
Running locally (Mac) as described at https://docs.openmicroscopy.org/omero/5.6.0/developers/Web/Deployment.html#using-wsgi and with 3 viewer tabs open, I don't see any thread increases using

$ pstree | grep gunicorn | wc
       7     132    1903

but I get "Internal Server Error" pages quite quickly without this PR.

With this PR, everything continues to work fine.
👍

@mtbc
Copy link
Member

mtbc commented Mar 17, 2020

Thank you all for your efforts here.

@mtbc mtbc merged commit 82105fa into ome:master Mar 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants