Bug 34193 – webkit-patch: Hangs on bad bugzilla username and completes?

• Component: Tools / Tests
• Status: REOPENED

[Chris Jerdonek]

Looking at the webkit-patch issues, this one is probably already recorded somewhere.

I had an experience where webkit-patch hung for a minute and terminated -- apparently because the bugzilla username in my keychain was out of date.  I spent another 15 minutes trying to get webkit-patch to work, only to discover later that the commit had already happened!  It looks like it got interrupted while trying to mark the bug resolved.  It's bad because there is no line of output saying anything like "Committed revision...." -- only "Processing patch 47411 from bug 34060."  It's hard to know this means "committed" because the next output line says it is parsing the Changelog.  It seems like the ChangeLog would be parsed before it starts committing.  It would also be good if the error says what remaining steps didn't complete, so the user knows what remaining steps need to be manually completed.

See below:

$ WebKitTools/Scripts/webkit-patch land-from-bug --no-build --ignore-builders 34060
Fetching: https://bugs.webkit.org/show_bug.cgi?id=34060&ctype=xml
1 reviewed patch found on bug 34060.
Processing 1 patch from 1 bug.
Cleaning working directory
Updating working directory
Processing patch 47411 from bug 34060.
Parsing ChangeLog: WebKitTools/ChangeLog
Reading Keychain for bugs.webkit.org account and password.  Click "Allow" to continue...
Logging in as [e-mail]...
Traceback (most recent call last):
  File "WebKitTools/Scripts/webkit-patch", line 108, in <module>
    WebKitPatch().main()
  File "/Users/chris_g4/Dev/Apple/WebKit-svn/WebKitTools/Scripts/webkitpy/multicommandtool.py", line 299, in main
    return command.check_arguments_and_execute(options, args, self)
  File "/Users/chris_g4/Dev/Apple/WebKit-svn/WebKitTools/Scripts/webkitpy/multicommandtool.py", line 113, in check_arguments_and_execute
    return self.execute(options, args, tool) or 0
  File "/Users/chris_g4/Dev/Apple/WebKit-svn/WebKitTools/Scripts/webkitpy/commands/download.py", line 116, in execute
    self._process_patch(patch, options, args, tool)
  File "/Users/chris_g4/Dev/Apple/WebKit-svn/WebKitTools/Scripts/webkitpy/commands/download.py", line 135, in _process_patch
    self._main_sequence.run_and_handle_errors(tool, options, state)
  File "/Users/chris_g4/Dev/Apple/WebKit-svn/WebKitTools/Scripts/webkitpy/stepsequence.py", line 66, in run_and_handle_errors
    self._run(tool, options, state)
  File "/Users/chris_g4/Dev/Apple/WebKit-svn/WebKitTools/Scripts/webkitpy/stepsequence.py", line 60, in _run
    step(tool, options).run(state)
  File "/Users/chris_g4/Dev/Apple/WebKit-svn/WebKitTools/Scripts/webkitpy/steps/closepatch.py", line 36, in run
    self._tool.bugs.clear_attachment_flags(state["patch"].id(), comment_text)
  File "/Users/chris_g4/dev/apple/WebKit-svn/WebKitTools/Scripts/webkitpy/bugzilla.py", line 632, in clear_attachment_flags
    self.authenticate()
  File "/Users/chris_g4/dev/apple/WebKit-svn/WebKitTools/Scripts/webkitpy/bugzilla.py", line 476, in authenticate
    "index.cgi?GoAheadAndLogIn=1")
  File "./autoinstall.cache.d/-7171706890285649816/mechanize-0.1.11.zip/mechanize-0.1.11/mechanize/_mechanize.py", line 209, in open
  File "./autoinstall.cache.d/-7171706890285649816/mechanize-0.1.11.zip/mechanize-0.1.11/mechanize/_mechanize.py", line 236, in _mech_open
  File "./autoinstall.cache.d/-7171706890285649816/mechanize-0.1.11.zip/mechanize-0.1.11/mechanize/_opener.py", line 191, in open
  File "/System/Library/Frameworks/Python.framework/Versions/2.6/lib/python2.6/urllib2.py", line 401, in _open
    '_open', req)
  File "/System/Library/Frameworks/Python.framework/Versions/2.6/lib/python2.6/urllib2.py", line 361, in _call_chain
    result = func(*args)
  File "./autoinstall.cache.d/-7171706890285649816/mechanize-0.1.11.zip/mechanize-0.1.11/mechanize/_http.py", line 756, in https_open
  File "./autoinstall.cache.d/-7171706890285649816/mechanize-0.1.11.zip/mechanize-0.1.11/mechanize/_http.py", line 706, in do_open
urllib2.URLError: <urlopen error [Errno 60] Operation timed out>
stonewall:WebKit-svn chris_g4$

[Joe Mason]

Created attachment 47530 [details]
patch to limit number of login attempts

As a first cut, lets cap the number of login attempts so that noninteractive logins don't get stuck if the credentials are wrong

[Adam Barth]

Comment on attachment 47530 [details]
patch to limit number of login attempts

Ok, but it seems like it should prompt you when the stored credentials don't work out.

[WebKit Commit Bot]

Comment on attachment 47530 [details]
patch to limit number of login attempts

Clearing flags on attachment: 47530

Committed r53976: <http://trac.webkit.org/changeset/53976>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Chris Jerdonek]

(In reply to comment #3)
> (From update of attachment 47530 [details])
> Clearing flags on attachment: 47530
> 
> Committed r53976: <http://trac.webkit.org/changeset/53976>

Thanks a lot for addressing this so quickly!

I mentioned a couple other things though--

> It's bad because there is no line of
> output saying anything like "Committed revision...." -- only "Processing
> patch 47411 from bug 34060."  It's hard to know this means "committed"
> because the next output line says it is parsing the Changelog.  It seems
> like the ChangeLog would be parsed before it starts committing.  It
> would also be good if the error says what remaining steps didn't
> complete, so the user knows what remaining steps need to be manually
> completed.

And Joe, you acknowledged that this is a "first cut".

Could you preserve the other suggestions in a report that is not marked RESOLVED (or alternatively, re-open this one), or explain why the commit makes it unnecessary to address the other suggestions?  Thanks a lot.

[Joe Mason]

I don't think I can reopen the bug, since I wasn't the submitter (at least, I don't see a link for that anywhere).  Can you file a new bug with a "depends on" field for this one, with the title something like "webkit-patch does not clearly state whether it committed or not" since the actual hang has been dealt with?  (Or else just reopen this one and change the title, if possible.)

This does point out a disturbing lack of atomicity - as I understand it, the patch was landed to svn ok, since your svn login was correct, but then the associated bug wasn't updated in bugzilla, since the bugzilla login was not.  Maybe it should login to bugzilla first and not push to svn unless that succeeded?  It could still fail if there's a timeout or bugzilla service outage, though

I guess the best approach would be, if any step failed, print out a nice big, "WARNING: the following steps were completed: ... But the following were NOT, you need to do them manually: ..."

[Chris Jerdonek]

(In reply to comment #6)
> I don't think I can reopen the bug, since I wasn't the submitter (at least, I
> don't see a link for that anywhere).  Can you file a new bug with a "depends
> on" field for this one, with the title something like "webkit-patch does not
> clearly state whether it committed or not" since the actual hang has been dealt
> with?  (Or else just reopen this one and change the title, if possible.)
> 
> This does point out a disturbing lack of atomicity - as I understand it, the
> patch was landed to svn ok, since your svn login was correct, but then the
> associated bug wasn't updated in bugzilla, since the bugzilla login was not. 
> Maybe it should login to bugzilla first and not push to svn unless that
> succeeded?  It could still fail if there's a timeout or bugzilla service
> outage, though
> 
> I guess the best approach would be, if any step failed, print out a nice big,
> "WARNING: the following steps were completed: ... But the following were NOT,
> you need to do them manually: ..."

Thanks, Joe.  I'll go ahead and create new bugs, etc. for you right now.

In the meantime, you can ask Maciej (or maybe Eric or someone) to grant you the editbugs privilege so you'll be able to reorganize/edit bugs yourself in the future.

[Chris Jerdonek]

(In reply to comment #7)
> Thanks, Joe.  I'll go ahead and create new bugs, etc. for you right now.

See the two bugs this depends on for the two issues remaining before this report should be marked resolved.

[Adam Barth]

In general, we prefer to have one patch per bug.  Bugs are cheap, so you might as well open new ones instead of reopening old ones.

These are all great suggestions, by the way.  We're very interested in improving the error handling for webkit-patch.  That's one of it's major weak points at the moment.

[Chris Jerdonek]

(In reply to comment #9)
> In general, we prefer to have one patch per bug.  Bugs are cheap, so you might
> as well open new ones instead of reopening old ones.

I agree with one patch per bug.

In addition to reopening this one, I did open two new bugs.  I just reopened this one to serve as the master bug for the other two (since it is the original report of all the issues).  This report can be closed when the other two are resolved, without submitting further patches to this report.  Or you can close this now.  Whatever works best for you guys to keep track of the issues.  Thanks!

[Adam Barth]

It's certainly fine to keep this bug open as a meta bug.  I was just spamming with generic advice.

[Eric Seidel (no email)]

Is this still an issue?