Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix/rework wserver (Issue 1133) #1164

Closed
wants to merge 11 commits into from
Closed

Fix/rework wserver (Issue 1133) #1164

wants to merge 11 commits into from

Conversation

TitiFix
Copy link
Contributor

@TitiFix TitiFix commented May 24, 2021

Fix/rework wserver/gwd/gwsetup for Windows from commit a6d35c7
(Unix specific functions are isolated to preserve fork operation)

Solved Issues :

  • BUG #1133 Gwd se plante après une demi minute
  • BUG #1131 Bad base name triggers "empty page" error response
  • BUG #1118 GeneWeb7 CGI Debian 10 - Verbose on every page

Summary :

  • connections without traffic are ignored (and closed after -conn_tmout timeout) -- windows version
  • taken into account possible abort / reset connection (many cases)
  • Server resiliency : exception handling is improved to log errors to a file (gwd-error.txt in wd directory); If possible in response to the faulty request the error is display. In DEBUG mode (not in release version) the backtrace is always activated.
  • the operation of the windows server is in dual process (default) or single process (-noproc). -noproc works but requires checking that all global variables are in an initial state each time request.ml is called
  • For Windows, the dual-process mode works by exchanging files for requests (other than miscellaneous files). It is less efficient than the mono process mode.
  • In the absence of a native syslog server on Windows, a file taking into account the internal syslogs of Geneweb is created (syslog.txt in wd directory). The operation of syslog on Linux is not changed.
  • The operation of the server on Linux systems is not changed (forks are used). However, some changes affect both Linux / Windows environments:
    -- HTTP exchanges is now HTTP/1.1 as well as CGI exchanges (CGI/1.1)
    -- wserver is the master of the connections: Flush and close must not be done outside of wserver.
    -- Mutil.bench now use stderr (sdtout is reserved for CGI mode)
    -- The launch of a 2nd Geneweb server on the same port is now refused
    -- HTTP status 404 now generated on missing file if requested file is not present.
    -- HTTP status 405 (Not allowed) for unsupported methods (PUT, HEAD, etc ...).
    -- add missing HTTP header for the index page in request.ml and HTTP 404 when unknown basename
    -- replacement charset = iso-8859-1 by utf-8 in the response headers (not uniform everywhere).
    -- Fixed mime types, some were wrong / obsolete (font, etc ...)
    -- the -cgi option to force CGI mode is no longer needed (remains for test purposes). CGI mode now depends on the presence of the GATEWAY_INTERFACE env. variable defined by the CGI RFC.
    -- General files (.js, .css, ...) are now in public cache; Personal images are in private cache
    -- Always close HTTP connection (HTTP close header)
  • Add missing files(previously in server mode, in 7.00, the search returned an HTML page as if the 1st directory level was a base) :
    css/bootstrap.min.css.map
    js/bootstrap.bundle.min.js.map
    etc/webfonts/fa-regular-400.eot (need only for IE)
    etc/webfonts/fa-solid-900.eot (need only for IE)
    etc/webfonts/fa-brands-400.eot (need only for IE)
    etc/favicon.ico
    etc/apple-touch-icon-precomposed.png
  • For detection of error codes (SysError too imprecise), the use of stdlib is prohibited. Only the unix library is used. wserver now manage buffering (adjustable from 1400 oct to 64K with -buf_size option in debug mode);

Test environnement : 64 bits
OS :

  • Windows 10 2004 (Home & pro edition)
  • Debian 10.4 (server & CGI mode)
  • Mac ( see hgouraud )

Browsers :

  • Edge (Chromnium) 90.0.818.66,
  • Chrome 90.0.4430.212 (64 bits),
  • Firefox 88.0.1,
  • Firefox 78.10.1 ESR
  • IE 11 (Windows 10 2004)

Network trafic : check with Wireshark.

TitiFix and others added 7 commits April 3, 2021 08:50
Better handling of sexcolor in menubar (avoids failure when no spouse)
AS_OK: handle pedigree collapse when sosa_filter=on
Invert params in aa is_substr baaccc; add 'in' as equivalent to 'is_ubstr'
@hgouraud
Copy link
Collaborator

On my Mac, tested in server and CGI mode

Copy link
Contributor

@sagotch sagotch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this PR. I only had a quick look for now but this seems to be good work.

Here is my first (quick) review.

bin/wserver/wserver.ml Outdated Show resolved Hide resolved
bin/gwd/gwd.ml Outdated Show resolved Hide resolved
bin/gwd/gwdLog.ml Outdated Show resolved Hide resolved
@@ -912,7 +916,6 @@ let treat_request =
#endif
| _ -> incorrect_request
end conf base ;
Output.flush conf ;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is wrong with flushing here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did not remove all the flushes in the code (several places). Output.flush does nothing more (see gwd.ml line 17). Cleaning will have to be done later. Only gwd and wserver can flush and close connections.

bin/wserver/dune Outdated Show resolved Hide resolved
bin/wserver/wserver.ml Show resolved Hide resolved
loop_write 0
end;
loop ()
exit 0
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need explicit exit 0?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did not modify the original code on this point.
We are in the child process (pid = 0).
exit 0 allows not return to the infinite loop of server (line 641 WServer)
therefore necessary

exit 0
with
| Unix.Unix_error (Unix.ECONNRESET, _, _)
| Unix.Unix_error (Unix.ECONNABORTED, _, _) -> Unix.close t; exit 0
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same question: do we need exit 0?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same response (necessary)

bin/wserver/wserver.mli Outdated Show resolved Hide resolved
loop ()
with Unix.Unix_error (Unix.ECONNRESET, _, _) -> ()

let close_connection () =
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is wrong with this, and why is shutdown_noerr better?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

close_connection did several things
skip_possible_remaining_chars was a useless function that only read empty stream (caused unnecessary waiting time) . only a shutdown / close sequence is needed

@TitiFix
Copy link
Contributor Author

TitiFix commented May 25, 2021

in plugins/export/plugin_export.ml
close_connection should be removed (like flush)
The reason is that wserver should be the master of connection closings. It is unhealthy to have connection management outside wserver. By removing the flush / close, this will allow to go, later, to a more modern HTTP server.

end
else geneweb_server ()

let () =
#ifdef DEBUG
Printexc.record_backtrace true;
Sys.enable_runtime_warnings false;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you mean true? I picked (and corrected) these two lines here : 53d561e

@sagotch
Copy link
Contributor

sagotch commented May 28, 2021

A lot of things is done in gwdLog. Wouldn't this to commits be good enough? I prefer adding as less code as possible in the core of geneweb.

e01ccd482 gwd: log to stderr as default instead of gwd.syslog file
f177589d0 Made syslog optionnal (not enabled by default).

@a2line
Copy link
Collaborator

a2line commented May 28, 2021

Bonjour. Pas encore testé les logs mais : (rah mauvaise manip)

  • le serveur fonctionne sous Windows (7 ici) sur le master ;
  • aucun des divers problèmes qu'on a rencontré les semaines précédentes ne sont présents !

@a2line
Copy link
Collaborator

a2line commented May 28, 2021 via email

@sagotch
Copy link
Contributor

sagotch commented May 28, 2021

-log <stderr> should be fine (I mean: -log "<stderr>", or -log '<stderr>' or -log \<stderr\>, I have no idea about how to escape this uner windows)

@a2line
Copy link
Collaborator

a2line commented May 28, 2021

Thanks, -log "<stderr>" works!

@TitiFix
Copy link
Contributor Author

TitiFix commented Jun 4, 2021

PR retirée.

@TitiFix TitiFix closed this Jun 4, 2021
@sagotch
Copy link
Contributor

sagotch commented Jun 7, 2021

Les principaux correctifs on été integrés à la main, mais il reste pas mal de choses à voir sur cette PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants