Range check error.

[david_navigator 12]

This com port routine has started to throw a range check error for one customer, at line ReadFile(ComHandle, Str[1], Count, BytesRead, @Overlapped);

 

It's old code which has worked for years, but I'm guessing something in D10.4.2 is doing something differently as this customer is testing a 10.4.2 compiled version.

Are there any gotcha's screaming out - I have a feeling that it's probably something to do with the Str[1] parameter, but I'm not sure.

 

function Tcomportserver.ReadString(var Str : AnsiString; Count : Integer) : Integer;
 var
    Overlapped : TOverlapped;
    BytesRead :  DWORD;
 begin
    SetLength(Str, Count);
    FillChar(Overlapped, SizeOf(Overlapped), 0);
    Overlapped.hEvent := CreateEvent(nil, True, True, nil);
    ReadFile(ComHandle, Str[1], Count, BytesRead, @Overlapped);
    WaitForSingleObject(Overlapped.hEvent, INFINITE);
    if not GetOverlappedResult(ComHandle, Overlapped, BytesRead, False) then
       raise EWriteError.Create('Unable to write to port: ' + LastErr);
    CloseHandle(Overlapped.hEvent);
    SetLength(Str, BytesRead);
    Result := BytesRead;
 end;

Many thanks

[Rollo62 539]

Have you checked that ComHandle is valid and accessible ?

Maybe the hardware behind has died or changed.

 

Can you open and read the port with other tools ?

[Cristian Peța 107]

If Count is 0 then Str[1] will trow a range check.

[Remy Lebeau 1436]

5 hours ago, david_navigator said:

This com port routine has started to throw a range check error for one customer, at line ReadFile(ComHandle, Str[1], Count, BytesRead, @Overlapped);

Make sure that Count is not 0, otherwise Str[1] will report a range error if Range Checking is enabled.  I would use PAnsiChar(Str)^ instead to avoid that check.

 

Also, you are not checking ReadFile() for failure.

 

Try this instead:

function Tcomportserver.ReadString(var Str : AnsiString; Count : Integer) : Integer;
var
  Overlapped : TOverlapped;
  BytesRead :  DWORD;
begin
  Result := 0;
  if Count <= 0 then Exit;
  SetLength(Str, Count);
  FillChar(Overlapped, SizeOf(Overlapped), 0);
  Overlapped.hEvent := CreateEvent(nil, True, True, nil);
  if Overlapped.hEvent = 0 then
    raise EWriteError.Create('Unable to create overlapped event object: ' + LastErr);
  try
    if not ReadFile(ComHandle, PAnsiChar(Str)^, Count, BytesRead, @Overlapped) then
    begin
      if GetLastError() <> ERROR_IO_PENDING then
        raise EWriteError.Create('Unable to read from port: ' + LastErr);
    end;
    if not GetOverlappedResult(ComHandle, Overlapped, BytesRead, True) then
      raise EWriteError.Create('Unable to read from port: ' + LastErr);
    SetLength(Str, BytesRead);
    Result := BytesRead;
  finally
    CloseHandle(Overlapped.hEvent);
  end;
end;

 

[david_navigator 12]

@Remy Lebeau Thanks for that - seems to address quite a few issues the original developer overlooked.

 

I noticed that in the original code the developer was using

 

   WaitForSingleObject(Overlapped.hEvent, INFINITE);
    if not GetOverlappedResult(ComHandle, Overlapped, BytesRead, False) then

Whereas in your version you are using

 

    if not GetOverlappedResult(ComHandle, Overlapped, BytesRead, True) then

Do these do the same thing ?

[David Heffernan 2353]

5 minutes ago, david_navigator said:

Do these do the same thing ?

Doesn't the original code have a race?

[david_navigator 12]

41 minutes ago, David Heffernan said:

Doesn't the original code have a race?

I've no idea. It's one of those units found on the internet many years ago and seems to have been working fine up until now.

[David Heffernan 2353]

10 minutes ago, david_navigator said:

seems to have been

Maybe worth digging a bit deeper. There seem to be a good few discussions on SO. I'm probably talking nonsense about a race, but I also bet that wait + GOR isn't right.

Edited June 16, 2021 by David Heffernan

[Guest]

@david_navigator The exception is raised when Count = 0 passed, but it is little more complicated than that and the real cause is the bad design, so let me explain

Overlapped operations can be used like that code, but that is really will kill the real benefit from its real usage, also this is not a problem here at all, the design could used a simple blocking operation and yet the problem still there, the usage and depending on count in both cases will still raise that exception.

The logic seen in your code says count is either being calculated from previous received data or checked by some event or API, the first case is not hard to fix as it is can be classified as bug or corrupt data, the second even easier to fix once you understand that, many IO operation fail to estimate the data size that need to be processed, as example in socket operations checking the size of available data to receive is unreliable, means there is data hence may be an event had being fired, but the data length is in question, that event had been fired when something changed, you check for data length to receive and found it as 100 but if you do read/recv on 200 you might get 170 bytes, in not so rare cases the checked length might return 0 while there is >0 data length ready to be processed.

I hope that was clear, how to do it right?

By changing the logic to remove the dependency on that Count parameter and read as much as you wish or can.
In that code you provided, i think adding this line before SetLegnth line will solve your problem completely and depending on the rate of data it might be faster

  Count := 2000;   // or you can use something close to the maximum 64*1024 or the most well enough value 8*1024

The solution is to read as much data as you can per operation and split them later, this is right and efficient way.

[david_navigator 12]

2 hours ago, Kas Ob. said:

The logic seen in your code says count is either being calculated from previous received data or checked by some event or API, the first case is not hard to fix as it is can be classified as bug or corrupt data, the second even easier to fix once you understand that, many IO operation fail to estimate the data size that need to be processed, as example in socket operations checking the size of available data to receive is unreliable, means there is data hence may be an event had being fired, but the data length is in question, that event had been fired when something changed, you check for data length to receive and found it as 100 but if you do read/recv on 200 you might get 170 bytes, in not so rare cases the checked length might return 0 while there is >0 data length ready to be processed.

@Kas Ob. Thanks. I think that makes sense. The code that calculates Count is

 

function Tcomportserver.InQue : Integer;
 var
    Errors :  DWORD;
    ComStat : TComStat;
 begin
   if not FConnected then
   begin
   Result := 0;
   end
   else
   begin
    if not ClearCommError(ComHandle, Errors, @ComStat) then
       raise EComStatus.Create('Unable to read com status: ' + LastErr);
    Result := ComStat.cbInQue;
   end;
 end;

so I think what you're saying is that at the time InQue runs (count) there might be 100 bytes waiting to be read, but by the time ReadString gets called, that might be 150 bytes ?

 

It's probably never risen it's head as a problem because this code is simply processing short barcodes, so it's 10 bytes, then a pause whilst the human does something and then another 10 bytes etc,

 

I'll refactor it though and check that it still works as the user expects.

[Guest]

11 minutes ago, david_navigator said:

so I think what you're saying is that at the time InQue runs (count) there might be 100 bytes waiting to be read, but by the time ReadString gets called, that might be 150 bytes ?

That was an example, but your exception was raised because cbInQue was 0 to read while ClearCommError was success, in this one particular case that code will raise exception.

 

13 minutes ago, david_navigator said:

It's probably never risen it's head as a problem because this code is simply processing short barcodes, so it's 10 bytes, then a pause whilst the human does something and then another 10 bytes etc,

OK, so you want 10 bytes per operation, then i suggest read 100 and if there is only 10 then you will get 10 or more or even less in case something out of ordinary went down and these data was hold somewhere and two reads should be performed, in all cases the best design should be like this

1) a buffer with enough size and counter to indicate number of bytes with in that buffer

2) watch for signal read received

3) issue read with length length(buffer) - counter and place the data on buffer[counter], not from the start but from counter 

4) loop and process each 10 in that buffer, any left over move them to the beginning of buffer and adjust the counter.

5) notice that counter will never be more than 10-1 at this step while it might be any value less than Length(Buffer)

6) goto 2

 

Now the above is design will not cause overflow, but as i said you can adjust your code after you understand how I/O operations works, one thing to remember size's is unreliable for most in not all I/O operations, size's only a fact after an operation is done and you got a success OS call.

[Remy Lebeau 1436]

7 hours ago, david_navigator said:

I noticed that in the original code the developer was using ... Whereas in your version you are using ... Do these do the same thing ?

Yes, per the documentation:

Quote

If the bWait parameter is TRUE, GetOverlappedResult determines whether the pending operation has been completed by waiting for the event object to be in the signaled state.