Edgewall Software
Modify

Opened 11 years ago

Last modified 6 years ago

#9602 new defect

[PATCH] auto-reload fail by design

Reported by: anatoly techtonik <techtonik@…> Owned by:
Priority: lowest Milestone: unscheduled
Component: general Version: 0.11.7
Severity: minor Keywords: development, patch
Cc: leho@…, Thijs Triemstra Branch:
Release Notes:
API Changes:
Internal Changes:

Description

It is rather funny that nobody spotted it before, but.. autoreload option designed for development mode actually stops to work once you have a syntax error in your script and the script fails to load.

branches/0.11-stable/trac/util/autoreload.py@7889

How does this happen. The normal flow with —auto-reload is to start new process for Trac and keep current process solely for monitoring how child behaves. If the child exits with the error code 3, it is restarted. Child itself runs two threads - first it creates thread where a main function is executed. This function processes requests and as a side effect imports all required files filling sys.modules list (shared with main thread, because, well, everything is shared between threads). Main thread then goes into infinite loop to periodically check timestamps of files in sys.modules and exit with error code 3 if timestamp mismatches.

Now the problem. If there is a syntax error in .py file it is not added into sys.modules and will not be checked to restart child process when the error is fixed.

P.S. I've added it to 0.11.7, because I'd like to see any fix in 0.11. It will be handy for exploring plugins behavior when porting them to 0.12

Attachments (2)

9602.fix-autoreload-for-bad-plugins.diff (1.8 KB ) - added by anatoly techtonik <techtonik@…> 11 years ago.
autoreload fix
9602.fix-autoreload-for-bad-plugins.2.diff (1.9 KB ) - added by anatoly techtonik <techtonik@…> 11 years ago.
do not add files several times

Download all attachments as: .zip

Change History (17)

comment:1 by Christian Boos, 11 years ago

Well, as we know this since ages, we took the habit of not making syntax errors ;-)

Seriously, for the few occasions this happens, we just have to restart tracd (make server), at the appropriate time, i.e. after fixing the error.

If you have an idea about how to make Trac notice this by itself(?), then please provide us with a patch, otherwise this will likely become a wontfix.

Also, in general, there won't be any more change to 0.11-stable, unless someone takes over maintenance (osimons?).

comment:2 by anatoly techtonik <techtonik@…>, 11 years ago

An idea. When there is a Trac[loader] ERROR: Failed to load plugin from XXX - add this XXX to a list of extra files to be monitored.

What is the proper way to pass the list of extra files from main function to monitoring thread? I can try to create a patch that uses global variable, but they say global variables are evil.

comment:3 by lkraav <leho@…>, 11 years ago

Cc: leho@… added

comment:4 by anatoly techtonik <techtonik@…>, 11 years ago

Attaching patch for 0.11.7. Code Review link for those who knows http://codereview.appspot.com/2131042/

by anatoly techtonik <techtonik@…>, 11 years ago

autoreload fix

comment:5 by Christian Boos, 11 years ago

Better add to a set than repeatedly append to a list…

And don't you want to restrict that to SyntaxError exceptions?

in reply to:  5 comment:6 by anatoly techtonik <techtonik@…>, 11 years ago

Replying to cboos:

Better add to a set than repeatedly append to a list…

set was added in Python 2.4 and I believe Trac 0.11 is still Python 2.3 compatible. This should not be a problem as the loader fails only once for every imported file.

And don't you want to restrict that to SyntaxError exceptions?

I am not sure that all errors that cause import failure are strictly syntax-related. For example, I get "Failed to load plugin" entry in trac.log for ZeroDivisionError.

by anatoly techtonik <techtonik@…>, 11 years ago

do not add files several times

comment:7 by anatoly techtonik <techtonik@…>, 11 years ago

Added check for file presence. Manually checked that ZeroDivisionError stops autoreloading mechanizm. I assume that any unhandled run-time exception during the import (and hence execution) of module will do this.

comment:8 by anonymous, 11 years ago

+1 for including this in the next release

comment:9 by Christian Boos, 11 years ago

?

comment:10 by anatoly techtonik <techtonik@…>, 11 years ago

Maybe we should make issue "stars" more visible?

comment:11 by Christian Boos, 11 years ago

Keywords: development added
Milestone: 0.13
Owner: set to Christian Boos
Priority: normallowest
Severity: criticalminor

Feels a bit hackish, but I'll give the patch a try.

comment:12 by anatoly techtonik <techtonik@…>, 11 years ago

There might be one more case not covered by this patch when autorefresh stops to work. When some .py file from plugins/ directory is removed (not necessary a plugin). This usually happens with test111.py scripts created to pinpoint some problem.

My high level assumption is that auto-reload fails, because of stacktrace or some exit condition in monitor thread. Perhaps intercepting this condition with obligatory output into trac.log will help to make less hackish patch.

comment:13 by Thijs Triemstra, 11 years ago

Cc: Thijs Triemstra added
Keywords: patch added
Summary: auto-reload fail by design[PATCH] auto-reload fail by design

comment:14 by Christian Boos, 11 years ago

Milestone: 0.13unscheduled

comment:15 by Ryan J Ollos, 6 years ago

Owner: Christian Boos removed

Modify Ticket

Change Properties
Set your email in Preferences
Action
as new The ticket will remain with no owner.
The ticket will be disowned.
as The resolution will be set. Next status will be 'closed'.
The owner will be changed from (none) to anonymous. Next status will be 'assigned'.

Add Comment


E-mail address and name can be saved in the Preferences .
 
Note: See TracTickets for help on using tickets.