Improve Apache docs

[jonashaag]

@jahir

While what you wrote here works: https://github.com/jonashaag/klaus/wiki/Apache---mod_wsgi-deployment/_compare/a759371%5E...a759371

It doesn't actually make a lot of sense to put these values into environment variables in the first place. We could simply directly call make_autoreloading_app with the config values.

[jahir]

I agree that using environment variables to communicate between to perl scripts is hacky and bad. But how would I import contrib.wsgi_autoreload without running the top level code (which stops with a KeyError because KLAUS_SITE_NAME is not set)? Would it suffice to wrap it into the usual if __name__ == "__main__":? (not sure if this works with WSGI)

[jonashaag]

Ah, good point, I'm not sure if the __name__ switch works. But maybe you can simply ignore the application object in that module?

[jahir]

Not sure what you mean by

ignore the application object

but anyway, I just tried wrapping the init stuff in wsgi_autoreload (with if __name__ == "__main__":) and now I can nicely use this wrapper script:

from klaus.contrib.wsgi_autoreload import make_autoreloading_app
application = make_autoreloading_app(
    "/srv/git/",
    "klaus on our git server",
    ctags_policy="tags-and-branches",
)

running it with uwsgi -w klaus.contrib.wsgi_autoreload --env KLAUS_SITE_NAME="Klaus Demo" --env KLAUS_REPOS_ROOT="/srv/git/" --http :8080 also works, so I guess we could change wsgi_autoreload. obvious downside is that this change would break the installation of everybody that uses wsgi_autoreload like my initial wrapper script (https://github.com/jonashaag/klaus/wiki/Apache---mod_wsgi-deployment/_compare/a759371%5E...a759371). Can we ignore this (probably not much people, if any, which could easily fix this) or should we put this into a new script?
Should I do a pull request for this?

[jonashaag]

Hm... Flask documentation says

Please make sure in advance that any app.run() calls you might have in your application file are inside an if __name__ == '__main__': block or moved to a separate file. Just make sure it’s not called because this will always start a local WSGI server which we do not want if we deploy that application to mod_wsgi.

http://flask.pocoo.org/docs/0.12/deploying/mod_wsgi/

Which is an indicator that __name__ != '__main__' with mod_wsgi, so this wouldn't work. On the other hand, the Flask documentation has that warning for uWSGI as well, and it seems to work for you anyways. So not sure what __name__ is set to for all these servers.

Not sure what you mean by
ignore the application object

Well, simply use from wsgi_autoreload import make_autoreloading_app and ignore that module's application object (but don't move it into a if __name__ block either). This requires that merely instantiating the application object does not have any side effects, of course.

[jonashaag]

The correct fix for all of this mess is to separate the core autoreload logic (

klaus/klaus/contrib/wsgi_autoreload.py

Lines 12 to 54 in 6f7f167

	# Shared state between poller and application wrapper
	class _:
	#: the real WSGI app
	inner_app = None
	should_reload = True
	def poll_for_changes(interval, dir):
	"""
	Polls `dir` for changes every `interval` seconds and sets `should_reload`
	accordingly.
	"""
	old_contents = os.listdir(dir)
	while 1:
	time.sleep(interval)
	if _.should_reload:
	# klaus application has not seen our change yet
	continue
	new_contents = os.listdir(dir)
	if new_contents != old_contents:
	# Directory contents changed => should_reload
	old_contents = new_contents
	_.should_reload = True
	def make_autoreloading_app(repos_root, *args, **kwargs):
	def app(environ, start_response):
	if _.should_reload:
	# Refresh inner application with new repo list
	print("Reloading repository list...")
	_.inner_app = make_app(
	[os.path.join(repos_root, x) for x in os.listdir(repos_root)],
	*args, **kwargs
	)
	_.should_reload = False
	return _.inner_app(environ, start_response)
	# Background thread that polls the directory for changes
	poller_thread = threading.Thread(target=(lambda: poll_for_changes(10, repos_root)))
	poller_thread.daemon = True
	poller_thread.start()
	return app

) from the launcher logic, i.e. have two different modules

[jahir]

Which is an indicator that name != 'main' with mod_wsgi, so this wouldn't work.

True (it is _mod_wsgi_LOTSOFHEX), but there's no harm, because you cannot use klaus.contrib.wsgi(_autoreload) directly from mod_wsgi (that's why I started to use a wrapper script in the first place). Or did I miss something?

Well, simply use from wsgi_autoreload import make_autoreloading_app

This would work, at least if

klaus/klaus/contrib/app_args.py

Line 11 in 6f7f167

os.environ['KLAUS_SITE_NAME']

would be changed to something like os.environ.get('KLAUS_SITE_NAME', 'poorly configured site') to prevent a KeyError on import (so the import is not done). But instantiating an application and then throwing it away is also ugly.

The correct fix for all of this mess is to separate the core autoreload logic [...] from the launcher logic, i.e. have two different modules

Yes, that's what I'd do if you think we should not change the existing wsgi_autoreload's behaviour.

[jonashaag]

Yes, that's what I'd do if you think we should not change the existing wsgi_autoreload's behaviour.

Agreed!

[jahir]

like this? (untested, maybe on monday)

[jonashaag]

Yep! If it works, I'm happy to merge that.

[stappersg]

FWIW: I wonder why this issue is open. From #198 do I understand that code was merged.

My wish: Update this issue with why it is open. (even better: close it )

[jonashaag]

Thank for the reminder!