Hi,

strictly Indy related, I tried registering on the official forums but apparently failed all attempts to answer the magic questions... perhaps I *am* a bot?

Anyway, this is a bug report. The following declarations are wrong:

First, in IdWinsock2.pas, "TAddrInfoExW = ADDRINFOEXA;". Should be ADDRINFOEXW of course. This is especially annoying since "PADDRINFOEXW = ^TAddrInfoEXW;", which then affects the ai_next member.

Second, in IdWship6.pas,
{code}
  {$IFDEF UNICODE}
  FreeAddrInfoEx : LPFN_FREEADDRINFOEX = nil;
  {$ELSE}
  FreeAddrInfoEx : LPFN_FREEADDRINFOEXW = nil;
  {$ENDIF}
{code}
This is the wrong way around. Fortunately not a show-stopper, the procedure name seems to be mapped correctly to the W variant so just a typecast is needed.

I've checked the latest development snapshot, they're present there too.

Cheers
- Asbjørn

> {quote:title=Remy Lebeau (TeamB) wrote:}{quote}
> Asbjørn wrote:
> 
> > The issue as far as I can see is that it's DoExecute always returns
> > true, instead of checking if it's still connected to the client.
> 
> I'm not sure why TIdEchoServer.DoExecute() is implemented the way it was. 
>  The think the following would be a better implementation:

Hey don't look at me :P As far as I can tell your code works just fine, and I agree it looks a lot cleaner.

I must admit I've never really gave the echo server much thought until now, writing some low-level socket code. 

Anyway, thanks again :)

- Asbjørn

Asbjørn wrote:

> A missing declaration this time, however it's required when using
> ConnectEx which is declared, so I'm considering this a bug (albet a
> minor one in the grand scheme of things :).
> 
> #define SO_UPDATE_CONNECT_CONTEXT   0x7010

Added.

--
Remy Lebeau (TeamB)

Asbjørn wrote:

> The issue as far as I can see is that it's DoExecute always returns
> true, instead of checking if it's still connected to the client.

I'm not sure why TIdEchoServer.DoExecute() is implemented the way it was. 
 The think the following would be a better implementation:

{code}
function TIdECHOServer.DoExecute(AContext: TIdContext): Boolean;
var
  LBuffer: TIdBytes;
  LIOHandler: TIdIOHandler;
begin
  Result := True;
  SetLength(LBuffer, 0);
  LIOHandler := AContext.Connection.IOHandler;
  LIOHandler.ReadBytes(LBuffer, -1);
  LIOHandler.Write(LBuffer);
end;
{code}

ReadBytes() will put the calling thread into an efficient sleep until new 
data arrives (the original code was actively polling for new data) and perform 
disconnect handling (ie, raise an exception if a disconnect occurs) while 
preserving the original semantics of echoing whatever raw data is available 
on the socket at the time of the read.

--
Remy Lebeau (TeamB)

> {quote:title=Asbjørn Heid wrote:}{quote}
> Anyway, this is a bug report.

A missing declaration this time, however it's required when using ConnectEx which is declared, so I'm considering this a bug (albet a minor one in the grand scheme of things :).

#define SO_UPDATE_CONNECT_CONTEXT   0x7010

The short version is, calls like shutdown() doesn't work properly unless you set this socket option when using ConnectEx.

See http://msdn.microsoft.com/en-us/library/ms737606(v=vs.85).aspx and http://www.lenholgate.com/blog/2005/05/winsock-connectex-shutdown-so-update-connect-context-and-wsanotconn.html for details. 

Cheers
- Asbjørn

> {quote:title=Asbjørn Heid wrote:}{quote}
> Anyway, this is a bug report.

Found another one. Not critical I suppose but still :)

The TCP Echo server enters an infinite loop consuming a full core for each client that connects, instead of properly disconnecting (ie OnDisconnect never fires).

The issue as far as I can see is that it's DoExecute always returns true, instead of checking if it's still connected to the client.

Here's my proposed change:
{code}
function TIdECHOServer.DoExecute(AContext: TIdContext): Boolean;
var
  LBuffer: TIdBytes;
  LIOHandler: TIdIOHandler;
begin
  Result := True;
  SetLength(LBuffer, 0);
  LIOHandler := AContext.Connection.IOHandler;
  LIOHandler.CheckForDataOnSource(50);
  if not LIOHandler.InputBufferIsEmpty then begin
    LIOHandler.InputBuffer.ExtractToBytes(LBuffer);
    LIOHandler.Write(LBuffer);
  end else begin
    Result := AContext.Connection.Connected; // BUGFIX
  end;
end;
{code}

After this change the Echo server's OnDisconnect event behaves as expected and there's no excessive CPU usage.

Cheers
- Asbjørn

Asbjørn wrote:

> Thanks again for the fixes. Any idea when these will be included in
> the official Delphi release? I'm writing some library code and I'm
> considering my options here: private declarations, require updated
> Indy or just next Delphi version.

These were interface changes, so Delphi has to wait until its next major 
version release before it can bundle them.  But you can update existing installations 
(subject to the restrictions mentioned in Indy's online install instructions).

--
Remy Lebeau (TeamB)

> {quote:title=Remy Lebeau (TeamB) wrote:}{quote}
> Asbjørn wrote:
> 
> > strictly Indy related, I tried registering on the official forums but
> > apparently failed all attempts to answer the magic questions...
> > perhaps I *am* a bot?
> 
> What exactly are you having a problem with?

It was, for some reason, not obvious to me what exactly the questions were asking, ie I was unsure what the proper response should be. Again, I'm not ruling out the posibility that I am a bot :P

> Fixed.

Thanks again for the fixes. Any idea when these will be included in the official Delphi release? I'm writing some library code and I'm considering my options here: private declarations, require updated Indy or just next Delphi version.

Cheers
- Asbjørn

> {quote:title=Remy Lebeau (TeamB) wrote:}{quote}
> Fixed.  However, SecurityQueryInfoLen is not optional, so using 'var' is 
> correct for it.

Ah yes, tired eyes saw too much, good catch.

Thanks for the prompt fixes.

Cheers
- Asbjørn

Asbjørn wrote:

> LPFN_GETADDRINFOEXA and LPFN_GETADDRINFOEXW, as well as the
> SET variants, all have the last parameter (lpNameHandle) as a var
> parameter. This must be a pointer because one *must* pass a nil
> pointer on Windows 7, Windows Server 2008 R2 and earlier[1]. So
> the correct declaration is "lpNameHandle : PHandle", without the var.

Fixed.

> edit: The GET variants also have an invalid declaration for ppResult.
> As the name suggests, it should be a pointer to a PADDRINFOEX. This
> isn't optional so should be "var ppResult: PADDRINFOEXW".

Fixed.

> Similar story with LPFN_WSAQUERYSOCKETSECURITY, except this time
> it's "just" optional parameters SecurityQueryInfo and
> SecurityQueryInfoLen.

Fixed.  However, SecurityQueryInfoLen is not optional, so using 'var' is 
correct for it.

--
Remy Lebeau (TeamB)