PB 4.30 - CloseNetworkConnection(clientID) bug?

[Joakim Christiansen]

Hello!

I've been messing around with network connections and threads making some FTP like program. And I've noticed that if I in my thread tries to close a connection that has already been closed by the client then my program will get an invalid memory access on the next "Select NetworkServerEvent()". I have really confirmed this with several tests (but I'm not able to put a snippet here yet).

So to fix it this is what I had to do:

Code: Select all

If GetClientPort(clientID) ;is it still open?
  CloseNetworkConnection(clientID)
EndIf

I also tried to check if it was still open with ConnectionID(clientID) but that would cause problems too. So what I suggest is just a command called IsConnection(clientID). Or that CloseNetworkConnection(clientID) should not cause trouble if the connection is already closed.

EDIT:
Move my thread all you want, but I still think this code is strange:
http://www.purebasic.fr/english/viewtop ... 524#286524
Why only IMA when used in the thread?

[freak]

Some code would help.

[AND51]

There is no code neccessary.
Everytime you try to access an invalid ConnectionID, you get an IMA.

No matter if this ConnectionID

• existed, but was closed
• never existed

It would be the best if there was a isConnection() command, as it was requested before. Sure, on the server side, you normally get informed about closed connections; but not on the client side. If a client tries to send data to a server or to close a connection to a server, it gets an IMA, if the server suddenly closes the connection.

Every other library has also an is***()-command.

[freak]

So its a feature request, not a bug.

• NetworkServerEvent() removes the ConnectionID when the client disconnects and you get the disconnect event, so you know the ID is now invalid
• NetworkClientEvent() never removes the ConnectionID (as there is no disconnect event on the client side), you have to call CloseNetworkConnection() there manually.

So there should be no case where you cannot know if the ID is valid.

[Joakim Christiansen]

So its a feature request, not a bug.

So getting an invalid memory access after trying to close an already closed connection is not a bug?

IsConnection() is a feature request yes; but I would not need it if it wasn't because of this bug.

I haven't been able to make a snippet to post here yet, maybe AND51 can help me, but I know there is something fishy going on. GetClientPort() works fine (no IMA) if the connection is closed, so whatever "protection" is used in that function should IMHO also be used in CloseNetworkConnection().

Not trying to sound angry here, I myself solved the problem by doing this:

Code: Select all

If GetClientPort(clientID) ;is it still open?
  CloseNetworkConnection(clientID)
EndIf

but I personally think CloseNetworkConnection() should have that safety inbuilt.

[freak]

> So getting an invalid memory access after trying to close an already closed connection is not a bug?

No. The same way as calling FreeMemory() on already freed memory is not a bug, or calling CloseWindow() on an already closed window, etc. etc.

GetClientPort() has no extra protection btw. It could crash just as well.

[Joakim Christiansen]

Well, Freak, at least I have an example now:

Code: Select all

EnableExplicit

Global clientId, serverId

Procedure test(d)
  CloseNetworkConnection(clientId)
EndProcedure

InitNetwork()
CreateNetworkServer(0,2222)
serverId = OpenNetworkConnection("localhost",2222)
Delay(1000)
If NetworkServerEvent() = #PB_NetworkEvent_Connect
  clientId = EventClient()
  Debug clientId
EndIf
CloseNetworkConnection(serverId)
Delay(100)

NetworkServerEvent() ;comment this line and no IMA?!
Delay(100)
CreateThread(@test(),0)
;CloseNetworkConnection(clientId) ;use this instead of thread and no IMA?!
Delay(1000)

Can you confirm this causing IMA when you test it too?
This code IS interesting... closing the connection from a thread will cause the IMA, but not if you just call CloseNetworkConnection(clientId) outside the thread. ALSO by commenting away NetworkServerEvent() it wont crash.

It just seems weird...

[AND51]

So there should be no case where you cannot know if the ID is valid.

Thanks for your explanation! It helps me a lot!

Well, I failed writing a code that shows my intention, but nevertheless you may have a look.
The following code crashed in 3.94 times, but with 4.30 I get no crash at all. That's strange (for me), because I expected the 3.94-behaviour. It must be my mistake then.

Server:

Code: Select all

InitNetwork()

CreateNetworkServer(0, 430)

Repeat
	Select NetworkServerEvent()
		Case 1
			Debug "Server: Incoming Connection "+Str(EventClient())
			Debug "Server: Connection will be terminated now by the server side"
			CloseNetworkConnection(EventClient())
	EndSelect
	Delay(50)
ForEver

Client:

Code: Select all

InitNetwork()

id=OpenNetworkConnection("127.0.0.1", 430)

Debug "Client: Connection initiated, ID="+Str(id)
MessageRequester("Client", "Server closed the connection (see code). Client tries to send data and to close connection, too. But this should crash now.", 48)
SendNetworkString(id, "How should I know if the connection is still alive?")
SendNetworkData(id, @"Is the connection still alive?", 30<<#PB_Compiler_Unicode)
CloseNetworkConnection(id)

Debug "You never see this line because of an IMA"

[AND51]

JLC, I tested your code and yes it crashes. I tell you why.
Every thread has its own ressources, so the main thread must call InitNetwork() and every single child thread, too.

But when I add a InitNetwork() to the thread, it doesn't work either. No matter, if Threadsafe is enabled or not. Somehow it must be possible to dsitribvute the ressources to different threads, I think. Because a connection, a file or a buffer is available program-wide. Or do I misunderstand something here?

Could you help us here freak?

// edit:
Please, see these related links about Threads + Ressources:
http://www.purebasic.fr/english/viewtop ... ressources
http://www.purebasic.fr/english/viewtop ... ressources

[freak]

> Every thread has its own ressources, so the main thread must call InitNetwork() and every single child thread, too.

Thats wrong. The init functions have to be called only once. If per-thread initialization is needed then the library will do that itself.

[AND51]

Ah, okay... Thats again a new information for me, thanks. Maybe, you can add this and similar techniques to the manual or write a blog entry about it? I think that would be helpful.

But then it should be possible to hand over ressources, such as RegExps, Files, Buffers, Connections, etc. right? I assume that this is not the case with JLCs code?

[Marlin]

So there should be no case where you cannot know if the ID is valid.

This does not seem to be true to me.

On the contrary: you almost never know if the connection is still valid.

It is not in the programmers influence (running a server), if or when a client disconnects.

You can only know about disconnects at the moment of polling NetworkServerEvent().

As this can bring up all kinds of events, not just for the one client connection,
I am wanting to close, this polling must take place in a part of the code
that is more general, not in a part handling one specific client request.

That general part can be made to make information about disconnects
available for the client request handling part (probably a thread),

but there is always a possibility that the disconnect takes place
after the last check for a NetworkServerEvent()!

In that case the program does not know, the client disconnected in the mean time,
tries to close the allready closed connection:

-> the program crashes (more or less silently)
-> your server is not running anymore!

My conclusion therefore is:

CloseNetworkConnection() should be safe to be called,
even if the client disconnected.
So should SendNetworkData().
(=program should never crash in such a case.)

I stumbled upon that problem,
running a http server, having the client download a larger file,
and than cancelling the download.

This scenario does not seem to be a big problem with windows,
where SendNetworkData() swallows even larger amounts of data
and returns immediately.
I can then free the buffer memory at once and even call
CloseNetworkConnection().
The Sending of the response with the file data is somehow
handled in the background and so is cancels by the client
or closing the connection at the right time.
It appears instant to the PB application and
it never finds out when the download is finished
or if the client canceled it.

With Linux, I have to call SendNetworkData() once in a while,
to get another chunk of data sent.
This does not like closed connections to much
(maybe it does get along with one attempt).
Here I see a risk of crash, if the clients aborts the download
at the wrong moment!
(If the server wants to close the connection as well)

The idea of a webserver going offline, because a client canceled
a download at the wrong moment, seems somewhat nasty to me.

Have I overlooked anything?