santigimeno / node-pcsclite

Bindings over pcsclite to access Smart Cards
ISC License
170 stars 127 forks source link

Handle last event before exit #46

Closed framillien closed 8 years ago

framillien commented 9 years ago

Always send last event (if any) before handling exit.

santigimeno commented 9 years ago

I think you're right in the case of pcsclite.cpp but not in cardreader.cpp. Notice that in https://github.com/santigimeno/node-pcsclite/blob/master/src/cardreader.cpp#L397 we send the last event.

One improvement for cardreader.cpp would be that in case of exit condition, don't perform 2 consecutive uv_async_send (https://github.com/santigimeno/node-pcsclite/blob/master/src/cardreader.cpp#L397 and https://github.com/santigimeno/node-pcsclite/blob/master/src/cardreader.cpp#L402) but just one, and then implement the modification you've proposed

framillien commented 9 years ago

Ok, going down threads, events, ... :smile: I discover lib_uv, but I have some interrogations:

santigimeno commented 9 years ago

@framillien OMG i didn't know about that warning. There is a problem then. We'll have to synchronize the threads to avoid calling uv_async_send consecutive times without having handled the callback. I'll also look into it. If we synchronize the threads correctly there should be no issue with using the same async struct.

santigimeno commented 9 years ago

Thanks for pointing that out

framillien commented 9 years ago

Hum ... I don't like locks ! Structure update must be securized, ok, but do not try to force sent of all callbacks, I think. We can loose some of them, important thing is to have the current state of readers. For me, this mean:

So, as you say, I think we can handle exit flag like you do it in pcsclite. I complete my patch with this.

santigimeno commented 9 years ago

@framillien I'm not to sure about that. I don't think the order is guaranteed by uv_async_send so maybe we don't received the last state of the readers

santigimeno commented 9 years ago

Hmm, maybe I'm wrong. By reading the libuv code I think it could be guaranteed. Not sure though :(

santigimeno commented 9 years ago

Yes I think it is, as the communication between threads is done via a pipe

framillien commented 9 years ago

Ok, I have pushed two new commits. In this version, the last status was sent, and on windows it's an error event. So I skip it, if it's related to card reader removed (like in pcsclite loop). In my tests cases, I never miss usefull events.

santigimeno commented 9 years ago

@framillien Apart from that comment it LGTM. I'll try to test it before merging it. Could you please squash it into one commit? Thanks!

Regarding the missing events. I'm not sure just getting the last one is always the best option. As an example: the case that a reader is disconnected and connected immediately and that the application misses the disconnected event. It could be that the application is designed to do something if the reader has been disconnected regardless the reader is connected again. The application won't be able to do that. I think the best option is to give the application every event pcscd emits so it's up to the application how to proceed. What do you think?

In any case, your patches makes things better.

framillien commented 9 years ago

Well, we are talking about thread synchronisation. If we start talking about human interaction, yes, I suppose that our event was handled by lib uv before the user can think about plugging his card again :wink:. Events we can eventually loose are flags like : card in use, ... For me, we can't do safe treatment based on this flags. But maybe someone could need it.

For now, I update the merge request.

santigimeno commented 9 years ago

LGTM. I'll try to merge it during the weekend. Thanks!!!

framillien commented 9 years ago

Thanks to you too.

santigimeno commented 9 years ago

@framillien I had merged your PR in version 0.4.3 (https://github.com/santigimeno/node-pcsclite/commit/3800c392918159fed582b2626591153725b29912) but had to revert it because it was causing crashes and unexpected error messages. One of the crashes happened when running the example.js with one reader with a card inserted. I think the problem lies in deciding when we should be emit the event before exiting the threads. For example, if the user cancels the checking for new readers via pcsc.close(), we should not be emitting the event but just exit. That's just one case, I'll look into it more thoroughly when I have time.

framillien commented 9 years ago

I see. Your test is on linux ? I also take a look begin of next week.

santigimeno commented 9 years ago

Yes. It fails both in Linux and OS X

framillien commented 9 years ago

Hello, I have commit a small update, but problem is still here, I try to check this week, sorry for delay.

framillien commented 9 years ago

Well, about our last events problems, I propose another option: based on m_state value. From code I see that 'm_state' could be:

So on event handler we can do:

I rework this branch like this ?

santigimeno commented 9 years ago

@framillien that sounds about right. Last weekend i tried that approach, and it almost worked. I didn't have much time, so I couldn't finish it. It would be great if you could. Thanks for taking the time to look into this

framillien commented 9 years ago

Ok, can you take a look and do some tests on this version ? I have done some tests with examples.js and our applications: It's look good for me.

I just have an interrogation about one exit condition (take a look in pcsclite.cpp), and sometime I receive an event twice when unplug a card reader (status 0xb), but I don't think it's a problem.

Thanks,

santigimeno commented 9 years ago

@framillien It looks great. My preliminary tests have been successful. Left some comments on the code. Thanks!

framillien commented 9 years ago

Ok, error and typo changed. I rebase to one commit ?

santigimeno commented 9 years ago

It looks good, but I'm getting some crashes from time to time due to some synchronization issues. Looking into it.

Disconnected
node: ../src/cardreader.cpp:73: virtual CardReader::~CardReader(): Assertion `ret == 0' failed.
Aborted
santigimeno commented 9 years ago

Got a fix for that

santigimeno commented 9 years ago

@framillien Could you give a try to https://github.com/santigimeno/node-pcsclite/tree/pr46? It has your changes with some fixes. Thanks!

santigimeno commented 9 years ago

@framillien Have you had time to test the branch?

santigimeno commented 8 years ago

@framillien did you get to test it? Thanks!

santigimeno commented 8 years ago

@framillien I included your changes here: https://github.com/santigimeno/node-pcsclite/commit/06d31638e241467c061f3688f1697fcce24b410a. They have been published in 0.4.6. Thanks!