I am not sure this is the fix we want. The intent of these assertions was to ensure that the file descriptor is not an invalid file descriptor in both send and recv.

Look at the receive code:

Here, we clearly check to avoid a case where the assert would trip unnecessarily.

The send code is a bit different.

Here it attempts to reopen a failed descriptor, checks status, and then persists another copy. This leads to a race condition because something else (e.g. recv) could close the descriptor between a successful open, and when the new copy is persisted.

This code should instead:

1. Call reopen
2. Safely persist a local copy of the descriptor.
3. Check for and return bad statuses
4. Check for fd == -1 and return DISCONNECTED

@vincewoo does this make sense?

We should also put back in the assert in the recv function.

@jsilveira1409 do you want to attempt the above change, or shall I?

@LeStarch hey thanks for the feedback! I'll give it a shot in the coming days and propose a solution! Thanks

It turns out this is a whole lot more complicated that first assumed. The crash comes from the fact that in the TcpServer re-connections are potentially a long process (wait listening for some unknown client).

We need this fix for the release today, so I am going to go ahead and make it as it affects multiple files.

For the time being, I will merge this PR as it will partially address the problem, and will give you credit.

We looked at this yet deeper, and decided this was the correct fix for now. A better fix would be to push re-connection requests onto their own thread, and get rid of this weird inter-threading logic.

However, for now this code results in an error when threads are reconnecting and haven't completed. This is the stop-gap solution.

I haven't had time to look more into it yet but will study it deeper when I do! Thanks for the merge!