开云体育

void EReader::stop() {
? if (m_hReadThread) {
? ? m_isAlive = false;
#if defined(IB_POSIX)
? ? if (!pthread_equal(pthread_self(), m_hReadThread))
? ? ? pthread_join(m_hReadThread, NULL);
#elif defined(IB_WIN32)
? ? if (!(GetCurrentThreadId() == GetThreadId(m_hReadThread)))
? ? ? WaitForSingleObject(m_hReadThread, INFINITE);
#endif
? }
}

?

Hey, if you feel that strongly about it and have the time then go ahead and make the pull request. In the worst case scenario it'll be good practice, a learning experience and software development lesson.

?袄冲(ツ)冲/?

Yeah, I see most of this as making a mountain out of a molehill. It strikes me, largely, as a misunderstanding; much like issuing a SIGKILL but expecting SIGTERM.

If you are familiar with unix signal processing you know that SIGKILL is immediately forced upon the process by the OS and the process my be left in an untidy state. SIGTERM, on the other hand, gives the process an opportunity to clean up and exit gracefully.

Since the API code is readily available, folks can implement their own version of EReader which suits their preference in this regard. Therefore the distinction becomes moot.

There may be some chance of acceptance for a minor pull request which, e.g., swaps the calls to the thread join and socket disconnect. But, even this could be seen as a dog chasing it's own tail since the argument would merely come down to the dislike of a default.

Moreover though, when you consider that the code for EReader hasn't changed in years... even a small change becomes unlikely. And, the suggestion of anything "major", like an intermediate "stop" method, becomes far fetched IMHO.

It's certainly an interesting nuance but probably better addressed by documentation alone. At least we have this thread of conversation now for reference :-)

I like evolution, so I may be wrong but here I am not sure I would flag this as a bug.
Just out of curiosity what undesirable side effect the current implementation generate? ?

Some food for thoughts:
Theses are tricky parts as it require tracking all threads together to do proper analysis. again I won't pretend I know all the implications.
I use it in C++/Win32 and it work really fine and fast (async) without any issues that worth a change from my perspective (IMHO).

Because:
1- If I ask a "disconnect" it's to be acted upon immediately I don't want any new data to be processed.
Exiting gracefully and fast a mutithread application can become a tricky process;
I am ready to live with recv receiving remnant of previous telco. It's a philosophical/aesthetic issue I agree,
in an ideal world, we are 100% sure that the last transaction was completed. yes. but ... we are not (at least I am not ) in this ideal world.

Also the "error" system of IB have to be seen as a client messaging service Same for WIN32 where GetLastError() is sometime used as a signal (Example when looking for enumeration of files, this is the only way to get a "no more file" info.)
The OS handle gracefully zombie packet on close socket, so no harm. (surely not the only apps that do that).
Codes are used to that, just look at handleSocketError you see a hard errno = 0; typical of way to reset it without concerns for unhandled error.

2- Related to #1 above, Putting the WaitForSingleObject(m_hReadThread, INFINITE); before eDisconnect may execute a last putMessageToQueue.
While the basic supplied code will do a hard beak, the while() loop with m_buf.size==0 , exit because m_isAlive==false, without executing anything else.

3- Notice that a EReader object is used during connection as a limited life (scoped) heap object to fetch server protocol revision in a synchronous mode, and to call startApi()
(I am not aware of what this proc? does but by the name of it I would avoid missing this call) this looks like a "trick" to overcome an un-anticipated evolution of the protocol,
See EClientSocket::eConnectImpl(....)
Actions happening in the dctor are then to be taken carefully.

I would agree that the inversion of code lines you suggested might not affect the behavior at it's synchronous, (m_hReadThread=0) , that you ought to get the protocol revision and I don't even see how you can abort this single message operation anyway. (aside of a socket shutdown) never see a need.

As buddy point it out:
- it show that playing with the dctor of EReader may have side effect.
In particular there are legacy code without a call to a ->stop(), ?? it's implicit in ~dctor and somewhere does the job.
- Just to mention, the m_signal used for this Connect EReader is shared by this ephemeral EReader, you most probably will use the same for your main EReader.? It need care, there are people who use more than one EReader on same m_signal

On Sun, Oct 1, 2023 at 03:15 PM, David Armour wrote:

I do not feel that destroying the EReader object is the correct way to disconnect.

IDK if this is what made you uneasy or what you meant, but maybe you'd prefer to see the call to eDisconnect happen in readToQueue as the penultimate action (i.e. before the final signal is issued)? In retrospect this makes a bit more sense to me.

On Sun, Oct 1, 2023 at 03:15 PM, David Armour wrote:

I agree with everything you said although I am not entirely happy with my final solution. I do not feel that destroying the EReader object is the correct way to disconnect. It is certainly not intuitive which means I cannot be the only person finding this problem. Is everybody using C++ just ignoring this error?

You may see this as a wart but I, personally, won't comment with regard to how intuitive things are or any design aesthetic. I will say that consideration should be given to backward compatibility and just how much existing code would break (and therefore need to change) if the disconnect where removed from the destructor.

Just some food for thought... since there's usually ripple effects, unintended consequences, and API stability is actually very important. Also, I don't think it's wise to hold off fixing some simple, obvious, and existing issue merely because there's an intention of addressing it in a more significant re-design. As they say, there is no time like the present and I appreciate small and tractable, incremental changes.

However, this may come down to a judgment call. Since, I guess, people have indeed been ignoring it or working around it (or it's simply gone unnoticed). After all, it only happens late in the game when there usually isn't much network activity expected or necessarily happening.

Anyway, I will sleep on it one more night before submitting a bug report.

Is there a special place to report API bugs or do we just raise ticket as usual?

A pull request is probably the best way to go about this:

On Sun, Oct 1, 2023 at 10:02 AM, David Armour wrote:

Thanks for your comment Buddy.

You're welcome. I think you've found an interesting oversight.

I agree that the logic would seem that setting m_isAlive to false during the ~EReader() call should prevent the code trying to read from the socket.

Yeah, superficially and at first blush it would seem that way. But it'd be a mistake to think so. I can understand how that initial thought may have fooled the original author into thinking the code was correct though. All this is post-mortem speculation on my part however.

Nevertheless, once considered more thoughtfully, it becomes apparent that nothing prevents the destructor from being called while the processing thread's readToQueue is in the while block.

The std::atomic will only insure that reading and writing m_isAlive from multiple threads is well-defined; but guarantees nothing w.r.t. the destructor and what's happening in the while loop.

I tried playing around with that. If I pause after m_isAlive is set to false, then continue, the code works successfully without 509 error. It is as if at

"Pause", with a sleep or something? No... that's surely a game of whack-a-mole. It might work sometimes, depending on unpredictable factors, and might not.

It can be fun to experiment with the timing while you investigate though. You can also try playing w/ ordering by running the code on a single core/cpu and see how that affects things.

full speed the atomic operation is not being completed before the call to eDisconnect() happens. On Visual Studio this even happens when I turn off all Optimisations. As eDisconnect() does not use anything related to that atomic, I wonder if the compiler is parallelising the operation as all atomic operations are very slow.

Compiler optimizations fall somewhat into the category of "unpredictable factors" I alluded to (although they'd be deterministic given the compiler source and enough work). It's a bit surprising that a critical section wasn't implemented via mutex, but maybe the author wanted to avoid a performance penalty.

I also noticed that eDisconnect() sets m_fd = -1.? This should prevent the call to processNonBlockingSelect() from trying to call onReceive() which ultimately triggers receive() and recv() causing the error. Again the setting of the atomic variable is not being done in time.

I am still scratching my head over this. I suspect some synchronization issue between the various threads.

Anyway... I think you found a legitimate problem and I wouldn't worry about it too much. I like your proposed solution as well. Joining the thread and then disconnecting makes clear sense to me. And, as long as m_pClientSocket isn't being shared all over the place I don't see a reason to use more complicated locking via mutex.

If I were the reviewer I could see signing off after some confirmation tests... open a ticket and see what they have to say. If you submit a patch remember to change the IB_POSIX ifdef block too :-)