daokoder / dao-modules

Dao Standard Modules
http://daovm.net
12 stars 5 forks source link

Switch the backend of web.http from Ciwetweb to Marten? #57

Closed daokoder closed 9 years ago

daokoder commented 9 years ago

In the recent issue #56, I found that the problem occurs when there is no Content-Length: in the response header. I begin to believe it is a Civetweb bug, after I reproduced the bug by removing the Content-Length: part from civetweb/examples/post.c. And interestingly, Mongoose/Marten has no such problem.

When I made the switch from Marten to Civetweb, I worried that the later could actually be less stable than Marten, given that I already had experienced an issue after the switch. The issue was that I cannot create a hidden window to get an OpenGL context within the request handle, but I wasn't sure if it was really a bug of civetweb, since creating gui window in non-main thread has always been tricky in many gui library.

More surprisingly, the server can support persistent connection (with enable_keep_alive option) without problem when compiled with Marten!

I am wondering if we should switch back to Marten, after all, we haven't experience any real problem with it yet. Another reason is that, the code size has doubled from Mongoose to Civetweb, yet I don't see any significant feature that Civetweb has over Mongoose/Marten.

dumblob commented 9 years ago

Regarding Content-Length:, it's a "feature", not a bug :cry:. I have no idea though why this requirement is there (I didn't read RFC standard yet).

daokoder commented 9 years ago

Regarding Content-Length:, it's a "feature", not a bug :cry:. I have no idea though why this requirement is there (I didn't read RFC standard yet).

You missed the point. The issue here is that, Content-Length is required even when keep alive is not enabled.

dumblob commented 9 years ago

Then it's a bug and we should report it (it seems, there were no issues with Content-Length: header so far).

Night-walker commented 9 years ago

Maybe we were too hasty, me in particular. But I wonder if Marten actually handles POST correctly: I think I tried it earlier with sample_post.dao and saw only the form with no variables appended.

dumblob commented 9 years ago

I'm still more inclined to Civetweb, because I believe that the 3 bugs we found (described in the first post) can be fixed. Do you dare to report them?

daokoder commented 9 years ago

But I wonder if Marten actually handles POST correctly: I think I tried it earlier with sample_post.dao and saw only the form with no variables appended.

Just tried, it works.

Maybe we were too hasty, me in particular.

No one to blame. The problem was that we didn't establish guidelines for switching third-party libraries. Maybe we should apply the following guidelines in the future:

Of course, if the new library has significant features over the current one, we can also consider to switch while still applying the third guideline about regression testing.

I'm still more inclined to Civetweb, because I believe that the 3 bugs we found (described in the first post) can be fixed.

The question is not that if these particular bugs can be fixed or not (and I think the one about hidden window creation is a tough one). The question is whether Civetweb is stable enough. The fact that we discovered these bugs so quickly, hardly makes Civetweb a convincing alternative (for now at least).

Another thing is that Civetweb has doubled the code size over Mongoose in about one year, I worry that its development goal may have diverged too much from Mongoose, and worry more that it might diverge further. Also, more code means more potential bugs.

I think we should switch back to Marten for now, and sticks to it as long as it works and does not have serious flaws that are beyond our expertise fixed. Also, I expect there will be some contributors to Marten, once more people start to use Dao and web.http etc., so we might get some help to improve and maintain Marten. This way, we will have more control on the direction of Marten.

Of course, I will not object to switching to Civetweb, when it gets really stable without getting too bloated.

dumblob commented 9 years ago

Understood. The guidelines are fitting - I would just change "These flaws are beyond our expertise to fix;" to "These flaws are beyond our expertise and time to fix and maintain;". Let's switch back to Marten and report the 3 found bugs to Civetweb repository and maybe raise questions about their vision of stability/maintenance.

Night-walker commented 9 years ago

I think we should switch back to Marten for now, and sticks to it as long as it works and does not have serious flaws that are beyond our expertise fixed. Also, I expect there will be some contributors to Marten, once more people start to use Dao and web.http etc., so we might get some help to improve and maintain Marten. This way, we will have more control on the direction of Marten.

I don't object. Besides, if/when Dao gets generic http support, current web.http can be put back to a separate repository with the status of external module, which wouldn't require as much scrutiny as a standard module.

daokoder commented 9 years ago

The bug related to the omission of Content-Length is now reported in the Civetweb project (https://github.com/bel2125/civetweb/issues/161). But the issue with hidden window creation (using GLFW) is not very convenient to reproduce without using Dao modules, and I am not entirely sure it is an issue of Civetweb, so I will not report it now.

daokoder commented 9 years ago

Besides, if/when Dao gets generic http support, current web.http can be put back to a separate repository with the status of external module, which wouldn't require as much scrutiny as a standard module.

I hope that day will come:)

bel2125 commented 9 years ago

Hello,

CivetWeb is stable and in running in several commercial applications since more than a year. I usually fix bugs within a week.

From your discussion here, from https://github.com/daokoder/dao-modules/issues/56, and the bug you reported (https://github.com/bel2125/civetweb/issues/161), I read you found problems when not setting the Content-Length field for a message. However, according to the HTTP specification (http://www.w3.org/Protocols/rfc2616/rfc2616-sec4.html#sec4.4), there are only cases where you are allowed to omit the Content-Length field - it is not really optional (http://stackoverflow.com/questions/14758729/http-post-content-length-header-required - neither for GET nor for POST). If you do not know the Content-Length before starting to send the response, use chunked transfer encoding - this is one of the many extensions made since the "mongoose era" (see https://en.wikipedia.org/wiki/Chunked_transfer_encoding). Instead of Content-Length: <number>, add Transfer-Encoding: chunked to the header (https://en.wikipedia.org/wiki/List_of_HTTP_header_fields#transfer-encoding-response-header). Then an arbitrary number of chunks with mg_printf("%x\r\n%s\r\n", strlen(chunk), chunk);, then mg_printf("0\r\n\r\n);.

One of the changes since mongoose was also to check all responses from the server with a HTTP protocol analyzer (I used "fiddler2" for this purpose). There were several protocol violations before, which have been fixed in civetweb. I do not really know what your project (dao-modules) is all about, but if it it has a web interface, I would recommend checking the web interface with a HTTP protocol analyzer.

Regards, bel

Night-walker commented 9 years ago

Thanks to your attention and detailed explanation. But I think it is not the case. In the tested scenario, the POST request header does include Content-length field, as it indeed should. But the response is not obliged to specify it. From RFC 7230, 3.3.3 (Message Body Length), which applies to the discussed case:

...
7.  Otherwise, this is a response message without a declared message
       body length, so the message body length is determined by the
       number of octets received prior to the server closing the
       connection.
daokoder commented 9 years ago

Hi bel, thank you for your prompt response and explanation.

One of the changes since mongoose was also to check all responses from the server with a HTTP protocol analyzer (I used "fiddler2" for this purpose). There were several protocol violations before, which have been fixed in civetweb.

Good to know this.

If you don't mind my asking, what are the changes/enhancements done on the multi-threading support. I am interested on this, because it might have something to do with the hidden window creation (using GLFW on Mac) problem.

I do not really know what your project (dao-modules) is all about, but if it it has a web interface, I would recommend checking the web interface with a HTTP protocol analyzer.

dao-modules is the repository for the standard modules of the Dao programming language (http://daovm.net). Among the modules, there is web/http to provide HTTP server and client functionalities.

Night-walker commented 9 years ago

Besides, if/when Dao gets generic http support, current web.http can be put back to a separate repository with the status of external module, which wouldn't require as much scrutiny as a standard module.

I hope that day will come:)

Maybe that day is closer then you think :) I've just committed some basic http handling functionality which can further be used to orchestrate higher-level connection handling.

bel2125 commented 9 years ago

The changes in multi-threading are different for different operating systems. CivetWeb internally uses the pthread API which is not available for Windows, so it uses it's own phread replacement functions. These functions did not work properly before, so the phread replacement functions have been completely rewritten. Now they use the thread local storage as a base for the pthread functions. I don't have a Mac, so I can not test it directly. CivetWeb should set a thread name for all threads now, also on Mac OS. The timeout handling has been rewritten for all systems, and the way the threads are stopped when the server closes.

Maybe there are a few changes more, I do not remember in the moment ... it's somewhat hard to say if I do not really know what to look for.

daokoder commented 9 years ago

Maybe that day is closer then you think :)

Indeed closer than I thought, though I did guess you would likely be the first one to do something about it :)

I've just committed some basic http handling functionality which can further be used to orchestrate higher-level connection handling.

Very good. When this net.http become mature enough, we can base web.http on it, if it is necessary to keep two. (I think it is necessary, since it would be more flexible to have net.http to handle just core functionalities such as HTTP protocol etc., web.http to handle high level things such as session management etc.)

daokoder commented 9 years ago

The changes in multi-threading are different for different operating systems. CivetWeb internally uses the pthread API which is not available for Windows, so it uses it's own phread replacement functions. These functions did not work properly before, so the phread replacement functions have been completely rewritten. Now they use the thread local storage as a base for the pthread functions. I don't have a Mac, so I can not test it directly. CivetWeb should set a thread name for all threads now, also on Mac OS. The timeout handling has been rewritten for all systems, and the way the threads are stopped when the server closes.

Maybe there are a few changes more, I do not remember in the moment ... it's somewhat hard to say if I do not really know what to look for.

Thanks for the detailed explanation. When I have time, I will try to dig a little bit into the changes to see what could have caused the issue for hidden window creation.

bel2125 commented 9 years ago

Did you have time to try the latest version from ~12 hours ago?

daokoder commented 9 years ago

Just tried, it's working now on Mac.

bel2125 commented 9 years ago

While the server is successfully used in different applications, I am not sure if this is the case with the mg_download client function. First, CivetWeb has only a part of the client functionality of well known dedicated client libraries like curl and wget, so I guess they are preferred as clients. But the old mg_download function (from Mongoose) is also very limited, so there is a new client API for CivetWeb: see https://github.com/bel2125/civetweb/issues/161#issuecomment-121058287. The new API is able to handle all kinds of requests from client to server.

Of course it is up to you what library you want to use for HTTP support in your project. CivetWeb is actively maintained and developed. It is quite stable, and bugs are usually fixed quickly.

daokoder commented 9 years ago

Of course it is up to you what library you want to use for HTTP support in your project. CivetWeb is actively maintained and developed. It is quite stable, and bugs are usually fixed quickly.

As long as CevetWeb has reasonable compatibility with Mongoose, I still consider it a potential option and may switch to it if really necessary. I hope it will stay small and efficient regardless what improvements it might get.

bel2125 commented 9 years ago

CivetWeb should stay reasonable small. I will not promise full compatibility to the 2013 MIT Mongoose version, this would not be a good long term perspective. However, there will be reasonable replacements if an API changes.

You are welcome to submit all potential issues you find while testing CivetWeb, so I can have a look at them - regardless if you finally decide to use it or not.

daokoder commented 9 years ago

The switching was done a few days ago, I think this issue can be closed now.