This is an archive of the discontinued Mercurial Phabricator instance.

hgweb: reuse body file object when hgwebdir calls hgweb (issue5851)
ClosedPublic

Authored by indygreg on Apr 24 2018, 5:16 PM.

Details

Summary

An unintended side-effect of f0a851542a05 was that the request body
file object (which uses a util.cappedreader) was constructed twice
when hgwebdir called into hgweb. Since we attempt to read all remaining
data from this file object when Content-Length is defined and since there
were two instances of this object and the client supplied no additional
data to read, this resulted in deadlock.

The fix implemented in this commit is to reuse the request body file
object when it is passed from hgwebdir to hgweb.

A test demonstrating hg clone and hg push via hgwebdir has been
added. Without this patch, the test hangs when doing hg clone.
Surprisingly, this must mean that we have effectively no test coverage
of the wire protocol when run via hgwebdir.

Diff Detail

Repository
rHG Mercurial
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

indygreg created this revision.Apr 24 2018, 5:16 PM

A test demonstrating hg clone and hg push via hgwebdir has been
added. Without this patch, the test hangs when doing hg clone.
Surprisingly, this must mean that we have effectively no test coverage
of the wire protocol when run via hgwebdir.

I thought that hgwebdir testing is pretty light too, but there should be at least some additional coverage with subrepos. But maybe something is subtly different. For example, there's not an obvious difference between this new clone test, and this existing one:

https://www.mercurial-scm.org/repo/hg/file/8c8b6d13949a/tests/test-subrepo-recursion.t#l266

While there's no prefix on this clone, there is on in its subrepo.

tests/test-push-http.t
399

This needs to be excluded on Windows. Maybe I should just trap the error that gets raised, so that this loads but does nothing?

indygreg added inline comments.Apr 25 2018, 12:08 AM
tests/test-push-http.t
399

Oops. This was left over from my debugging. It can safely be removed.

queued for stable with showstack dropped

This revision was automatically updated to reflect the committed changes.