1230862 - Remove mozhttpd

Status: NEW

(Testing :: Mozbase, defect)

Description

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Per bug 1150512, it is unsupported.

Comment 1

[William Lachance (:wlach)]

Personally I don't see anything wrong with keeping it in the tree in its deprecated state, at least as long as there are things using it. I'm still happy to accept/review patches to mozhttpd that fix bugs, I just don't want to add any new features to it.

Comment 2

[Julien Pagès (:parkouss)]

I'm in favor of removing it, since it is deprecated. I can work on it.

Comment 3

[Julien Pagès (:parkouss)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=177e31f25215

Comment 4

[Julien Pagès (:parkouss)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=38793fe76afa

Comment 5

[Julien Pagès (:parkouss)]

Oups, I did not meant to show those try pushes;

Anyway, as you can see I started looked at it. It works fine as far as I see, I believe the only drawback is for running talos jobs as we need to make it use wptserve in-tree version.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=38793fe76afa

Should test that (already working fine locally).

Maybe a full try will help, to ensure nothing is broken - but I think that mozhttpd was mainly used for mozbase tests, which are run anyway with a basic push try.

Comment 6

[Julien Pagès (:parkouss)]

(In reply to Julien Pagès (:parkouss) from comment #5)

> Maybe a full try will help, to ensure nothing is broken - but I think that
> mozhttpd was mainly used for mozbase tests, which are run anyway with a
> basic push try.

I mean for things that can break try. mozhttpd was used for a lot of other things, here are my patches so far:

https://hg.mozilla.org/try/rev/177e31f25215
https://hg.mozilla.org/try/rev/38793fe76afa

Comment 7

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Maybe it might make sense to convert all existing project to wptserve first?

https://dxr.mozilla.org/mozilla-central/search?q=mozhttpd&redirect=true&case=false

Comment 8

[Julien Pagès (:parkouss)]

(In reply to Henrik Skupin (:whimboo) from comment #7)
> Maybe it might make sense to convert all existing project to wptserve first?
> 
> https://dxr.mozilla.org/mozilla-central/
> search?q=mozhttpd&redirect=true&case=false

This is done with the first patch, https://hg.mozilla.org/try/rev/177e31f25215, unless I am misunderstanding.

Comment 9

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Oh, never mind then! :)

Comment 10

[Julien Pagès (:parkouss)]

all green. I guess :wlach is a good person to ask for review for that, given that he is ok to remove mozhttpd. :)

Comment 11

[Julien Pagès (:parkouss)]

Attachment: MozReview Request: Bug 1230862 - remove mozhttpd dependency for Talos. r=jmaher

Review commit: https://reviewboard.mozilla.org/r/33259/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/33259/

Comment 12

[Julien Pagès (:parkouss)]

Attachment: MozReview Request: Bug 1230862 - replace mozhttpd with wptserve. r=wlach

Review commit: https://reviewboard.mozilla.org/r/33261/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/33261/

Comment 13

[William Lachance (:wlach)]

Comment on attachment 8714908 [details]
MozReview Request: Bug 1230862 - remove mozhttpd dependency for Talos. r=jmaher

https://reviewboard.mozilla.org/r/33259/#review30601

Two very minor changes that are more a matter of personal taste than anything else. This looks great! I'd be happy to see this land as-is, even better if my personal nits are addressed. :)

::: testing/mozbase/mozcrash/tests/test.py:169
(Diff revision 1)
> -        def get_symbols(req):
> +        @handlers.handler

I would add a newline before ```@handlers.handler``` here.

::: testing/mozbase/mozprofile/tests/test_preferences.py:370
(Diff revision 1)
> +                                    % httpd.httpd.server_address)

Note that important, but I think it makes more sense to put the % on the previous line.

Comment 14

[William Lachance (:wlach)]

Comment on attachment 8714909 [details]
MozReview Request: Bug 1230862 - replace mozhttpd with wptserve. r=wlach

https://reviewboard.mozilla.org/r/33261/#review30621

Comment 15

[Julien Pagès (:parkouss)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/200da85932e9d4ce3cf44ede8802ba2478a95b7d
Bug 1230862 - Remove mozhttpd. r=wlach

https://hg.mozilla.org/integration/mozilla-inbound/rev/8bbaec3ed0f0bef19cbd9370f1fac92ece43ff18
Bug 1230862 - use wptserve in-tree for talos. r=wlach

Comment 16

[Julien Pagès (:parkouss)]

I fixed the nits and landed the patches.

Comment 17

[Carsten Book [:Tomcat]]

backed this out seems this caused https://treeherder.mozilla.org/logviewer.html#?job_id=21414859&repo=mozilla-inbound which turned into a perma failure after this push

Comment 18

[Carsten Book [:Tomcat]]

seems https://treeherder.mozilla.org/logviewer.html#?job_id=21410135&repo=mozilla-inbound is also related to this push

Comment 19

[Pulsebot]

Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a4f070f35677
https://hg.mozilla.org/integration/mozilla-inbound/rev/a85a19451da8

Comment 20

[Julien Pagès (:parkouss)]

Thanks Carsten, and sorry for this.

Looks like it is a failure of talos g2 on windows 7 and 8 (maybe mac osx also):

https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=8bbaec3ed0f0&selectedJob=21409294

Other talos jobs and others platforms seems good. :jmaher, any idea ?

Comment 21

[Joel Maher ( :jmaher ) (UTC -8)]

I suspect after reading a few logs that we are running out of memory on the machine- hence causing firefox to hang.

Could we look at wptserve and see if there is a way to measure the memory of the python process while serving a bunch of memory.  Maybe compare before/after of the overall OS on a set of runs?

Comment 22

[Phil Ringnalda (:philor)]

Be careful about try runs - 10.10 g2 wound up putting something in the log that matches one of the strings that causes buildbot to auto-retry, so unless you cancel it it will infinitely retry.

Comment 23

[Julien Pagès (:parkouss)]

(In reply to Joel Maher (:jmaher) from comment #21)
> I suspect after reading a few logs that we are running out of memory on the
> machine- hence causing firefox to hang.
> 
> Could we look at wptserve and see if there is a way to measure the memory of
> the python process while serving a bunch of memory.  Maybe compare
> before/after of the overall OS on a set of runs?

Thanks Joel for the answer! Indeed, I wrote a new server from scratch, and this time g2 seems good:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=84d7aa647615&selectedJob=16587024

There is an issue with S on win7 though, but I suspect it is either an intermittent, or a bad base for my try (PROCESS_CRASH).

I triggered some more jobs, just to be sure. I'll ask you for a review if that works, will be https://hg.mozilla.org/try/rev/cd80c2cbef37 with a better commit message. Roughly a pure python webserver, ran in a new process instead of mozhttpd or wptserve.

Comment 24

[Joel Maher ( :jmaher ) (UTC -8)]

The only concern I have here is that we already have mozhttpd which works and then switching to wptserve failed and now we are just rewriting another thing from scratch.  It sounds like running in a new process is the big win here- to me that is critical and useful to write code from scratch.

This falls back to understanding the problem more, or maybe understanding what bug we are really solving here by not using mozhttpd.

:parkouss, there is a big old problem with 's' on windows right now due to some changes in new tab, if you were to push to try with no changes, you would see this as well.

Comment 25

[Julien Pagès (:parkouss)]

So, we looked at that in depth, and wptserve is doing a full read() on file, and returning whole file contents as byte string. It does so to be able to apply filters on the content, according to jgraham.

mozhttpd is using SImpleHttpServer under the hood (which I used here also), and this read content by chunks of 16k - so I think this is the issue for us.

It seems that we could write something like a SimpleFileHandler and hook that in wptserve. At this point I wonder if the server wrote from scratch here isn't a better alternative, since it is 5 lines of code and that talos needs for the web server are low (only static serving). Plus it is in a new process, which probably is better - but may/will alter numbers.

Comment 26

[Joel Maher ( :jmaher ) (UTC -8)]

thanks for the explanation.  I suggest we file bugs on wptserve and go forward with this implementation- it is simple and easy to hack on as needed.

Comment 27

[Julien Pagès (:parkouss)]

Comment on attachment 8714908 [details]
MozReview Request: Bug 1230862 - remove mozhttpd dependency for Talos. r=jmaher

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/33259/diff/1-2/

Comment 28

[Julien Pagès (:parkouss)]

Comment on attachment 8714909 [details]
MozReview Request: Bug 1230862 - replace mozhttpd with wptserve. r=wlach

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/33261/diff/1-2/

Comment 29

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 8714908 [details]
MozReview Request: Bug 1230862 - remove mozhttpd dependency for Talos. r=jmaher

https://reviewboard.mozilla.org/r/33259/#review31205

please just explain the open(os.devnull) in a comment, the patch looks great.  Just to confirm, this works with --develop mode?

::: testing/talos/talos/run_tests.py:75
(Diff revision 2)
> -    return mozhttpd.MozHttpd(host=host, port=int(port), docroot=here)
> +    with open(os.devnull, 'w') as null:

I am not familiar with open os.devnull.

Comment 30

[Julien Pagès (:parkouss)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d9f5b04d2b77

Comment 31

[Julien Pagès (:parkouss)]

Valgring build is busted, I have the feeling that it is for the same reason.

Comment 32

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee didn't login in Bugzilla in the last 7 months.
:gbrown, could you have a look please?
For more information, please visit auto_nag documentation.