[m-rev.] for review: improvements to net/tcp.m

[Peter Ross]

On Wed, Mar 28, 2007 at 03:50:36PM +1000, Peter Wang wrote:
> Improvements to the tcp module.
> 
> extras/net/tcp.m:
> 	Make tcp an instance of the `reader' typeclass with unit `string' and
> 	implement line reading efficiently in C.
> 
> 	Do input buffering on sockets so it's not necessary to call recv() for
> 	each character read.
> 
> 	Delete `is_eof' predicate.  This is no longer needed due to the change
> 	in the way characters are read.
> 
> 	Add predicates tcp__ignore_sigpipe and tcp__unignore_sigpipe to ignore
> 	SIGPIPE signals that are sent if writing to a broken socket.  Disabling
> 	SIGPIPE allows write calls to return an error code instead of aborting
> 	the process.
> 
> 	Fix an incorrect `will_not_call_mercury' annotation on
> 	`handle_shutdown'.
> 
> 	Fix memory leaks caused by calling MR_NEW instead of MR_GC_NEW.
> 
> 
> Index: tcp.m
> ===================================================================
> RCS file: /home/mercury1/repository/mercury/extras/net/tcp.m,v
> retrieving revision 1.1
> diff -u -r1.1 tcp.m
> --- tcp.m	16 Nov 2006 04:01:49 -0000	1.1
> +++ tcp.m	28 Mar 2007 05:41:41 -0000
> @@ -57,11 +57,23 @@
>  
>  :- instance input(tcp, io.state, tcp.error).
>  :- instance reader(tcp, character, io.state, tcp.error).
> +:- instance reader(tcp, string, io.state, tcp.error).
>  
You should state what sort of strings are read?
Until a new line, for something else.

It appears to be until a new line, you should state this and state
that the newline is in the string.

Also for me the error returned should also include the string that
has been read up until the error, when reading a string.

> @@ -376,18 +395,27 @@
>  :- instance input(tcp, io.state, tcp.error) where [].
>  :- instance reader(tcp, character, io.state, tcp.error) where [
>  	(get(T, Result, !IO) :-
> -		tcp.read_char(T ^ handle, C, B, !IO),
> -		( B = yes,
> -			Result = ok(C)
> -		; B = no,
> -			is_eof(T ^ handle, IsEof, !IO),
> -			( IsEof = yes ->
> -				Result = eof
> -			;
> -				get_errno(T ^ handle, Errno, !IO),
> -				Result = error(Errno)
> -			)
> -			
> +		tcp__read_char(T ^ handle, Char, !IO),
> +		( Char = -1 ->
> +			Result = eof
> +		; Char = -2 ->
> +			get_errno(T ^ handle, Errno, !IO),
> +			Result = error(Errno)
> +		;
> +			Result = ok(char.det_from_int(Char))
> +		)
> +	)
> +].
> +:- instance reader(tcp, string, io.state, tcp.error) where [
> +	(get(T, Result, !IO) :-
> +		tcp__read_line_as_string(T ^ handle, ErrCode, String, !IO),

I'm not sure this code handles the case of reading from a socket the
following "ABC", eg no newline before the socket closes.

> +		( ErrCode = -1 ->
> +			Result = eof
> +		; ErrCode = -2 ->
> +			get_errno(T ^ handle, Errno, !IO),
> +			Result = error(Errno)
> +		;
> +			Result = ok(String)
>  		)
>  	)
>  ].
> @@ -425,31 +453,94 @@
>  %-----------------------------------------------------------------------------%
>  %-----------------------------------------------------------------------------%
>  
> -:- pred read_char(tcp_handle::in, char::out, bool::out, io::di, io::uo)
> -	is det.
> +:- pragma foreign_decl("C", "
> +	/* Note: some Mercury code uses the -1 and -2 constants directly. */
> +	#define TCP_EOF	    -1
> +	#define TCP_ERROR   -2
> +
> +	int TCP_get_char(ML_tcp *sock);
> +").
> +
> +:- pragma foreign_code("C", "
> +	int TCP_get_char(ML_tcp *sock)
> +	{
> +		if (sock->buf_pos >= sock->buf_len) {
> +			/* Refill buffer. */
> +			int nchars = recv(sock->socket,
> +				sock->buf, sizeof(sock->buf), 0);
> +			if (nchars == SOCKET_ERROR) {
> +				sock->error = ML_error();
> +				return TCP_ERROR;
> +			} else if (nchars == 0) {
> +				return TCP_EOF;
> +			} else {
> +				sock->buf_pos = 1;
> +				sock->buf_len = nchars;
> +				return sock->buf[0];
> +			}
> +		} else {
> +			return sock->buf[sock->buf_pos++];
> +		}
> +	}
> +").
> +

...

> +:- pred tcp__read_line_as_string(tcp_handle::in, int::out, string::out,
> +	io::di, io::uo) is det.
> +:- pragma foreign_proc("C",
> +	tcp__read_line_as_string(TCP::in, ErrCode::out, Str::out,
> +		IO0::di, IO::uo),
> +	[will_not_call_mercury, promise_pure, thread_safe, tabled_for_io],
> +"
> +	ML_tcp *sock = (ML_tcp *) TCP;
> +	size_t	BufLen = 1024;
> +	off_t	BufPos = 0;
> +	char	*Buf;
> +	int	Chr;
> +
> +	Buf = MR_malloc(BufLen);
> +
> +	while (1) {
> +		Chr = TCP_get_char(sock);
> +		if (Chr < 0) {
> +			ErrCode = Chr;
> +			break;
> +		}
>  
> +		if (BufPos >= BufLen) {
> +			BufLen += 1024;
> +			Buf = MR_realloc(Buf, BufLen);
> +		}
> +		Buf[BufPos++] = Chr;
> +		if (Chr == '\\n') {
> +			ErrCode = 0;
> +			break;
> +		}
> +	}
> +
> +	if (ErrCode == 0) {
> +		if (BufPos >= BufLen) {
> +			BufLen += 1;
> +			Buf = MR_realloc(Buf, BufLen);
> +		}
> +		Buf[BufPos] = '\\0';
> +		MR_make_aligned_string_copy(Str, Buf);
> +	} else {
> +		Str = NULL;
> +	}
> +
> +	MR_free(Buf);
> +	IO = IO0;
> +").

This code doesn't do the right thing when the socket buffer has "ABC"
only in it.

--------------------------------------------------------------------------
mercury-reviews mailing list
Post messages to:       mercury-reviews at csse.unimelb.edu.au
Administrative Queries: owner-mercury-reviews at csse.unimelb.edu.au
Subscriptions:          mercury-reviews-request at csse.unimelb.edu.au
--------------------------------------------------------------------------