nyholku / purejavacomm

Pure Java implementation of JavaComm SerialPort
http://www.sparetimelabs.com/purejavacomm/index.html
BSD 3-Clause "New" or "Revised" License
363 stars 146 forks source link

read() on Windows might incorrectly block forever #47

Closed ytai closed 9 years ago

ytai commented 10 years ago

First of all - good job on getting a lot of the Linux / OSX quirks out of the way! This one feels a lot better. To me, especially the ability to cleanly exit a blocked reader when the port is closed was a pain and it seems to work flawlessly now. Previously to work around that issue, I would set a timeout on the reading and wrap the InputStream with a retry loop that would give up when closed. I was now going to get rid of this hack, but then ran into another problem: sometimes my read()s would block forever despite having pending data. I investigated a little and found a problematic race condition in JTermiosImpl (Windows). Apparently, you are aware of it, since the code and the comments indicated that you tried to work around the fact that Windows would only give you "new data" notifications, as opposed to "have data" (a common problem with edge-triggered vs. level-triggered events). If data arrives between the time you poll and the time you WaitForMultipleObjects, you'll miss the event. However, the way you've worked around it relied on the user-set timeout - you'll block the client way more than necessary, or even worse (such as in my case) - forever! I'm not a Windows API expert, so I cannot advise you on a cleaner solution as far as this goes. However, at least on the Java front you can do better:

deadline = now + user timeout do { if (poll for events) break timeout = min(MAX_LAG, deadline - now) WaitForMultipleObjects(..., timeout) } while (now < deadline)

// We will get here at most MAX_LAG time after an event occurred or deadline expired.

This is not very elegant, but at least obviously correct.

Keep up the good work!!!

nyholku commented 10 years ago

Hi, I've now had some time to study this and I'm not sure I understand the problem and nor how this fix should be applied. Can you elaborate a bit?

Or better yet, if this has bothered you, you probably have a patched version for your own work, care to share that?

ytai commented 10 years ago

Copying from an email I sent you a while back on the subject:

I'm working on a new release of the IOIO software and among lots of other cool stuff, have upgraded to PJC 0.0.21. First of all - good job on getting a lot of the Linux / OSX quirks out of the way! This one feels a lot better. To me, especially the ability to cleanly exit a blocked reader when the port is closed was a pain and it seems to work flawlessly now. Previously to work around that issue, I would set a timeout on the reading and wrap the InputStream with a retry loop that would give up when closed. I was now going to get rid of this hack, but then ran into another problem: sometimes my read()s would block forever despite having pending data. I investigated a little and found a problematic race condition in JTermiosImpl (Windows). Apparently, you are aware of it, since the code and the comments indicated that you tried to work around the fact that Windows would only give you "new data" notifications, as opposed to "have data" (a common problem with edge-triggered vs. level-triggered events). If data arrives between the time you poll and the time you WaitForMultipleObjects, you'll miss the event. However, the way you've worked around it relied on the user-set timeout - you'll block the client way more than necessary, or even worse (such as in my case) - forever! I'm not a Windows API expert, so I cannot advise you on a cleaner solution as far as this goes. However, at least on the Java front you can do better:

deadline = now + user timeout do { if (poll for events) break timeout = min(MAX_LAG, deadline - now) WaitForMultipleObjects(..., timeout) } while (now < deadline)

// We will get here at most MAX_LAG time after an event occurred or deadline expired.

This is not very elegant, but at least obviously correct.

Keep up the good work!!!

On Fri, Mar 28, 2014 at 12:53 AM, nyholku notifications@github.com wrote:

Hi, I've now had some time to study this and I'm not sure I understand the problem and nor how this fix should be applied. Can you elaborate a bit?

Or better yet, if this has bothered you, you probably have a patched version for your own work, care to share that?

Reply to this email directly or view it on GitHubhttps://github.com/nyholku/purejavacomm/issues/47#issuecomment-38895644 .

nyholku commented 10 years ago

@ytai … any progress with the testing of the version I sent to you…?

ytai commented 10 years ago

Apologies, no. I've had some crazy two weeks with at least one more ahead of me. Leaving this in my inbox with the hope of eventually getting around to it.

On Wed, Aug 27, 2014 at 9:35 AM, nyholku notifications@github.com wrote:

@ytai https://github.com/ytai … any progress with the testing of the version I sent to you…?

— Reply to this email directly or view it on GitHub https://github.com/nyholku/purejavacomm/issues/47#issuecomment-53601754.

nyholku commented 10 years ago

No need to apologise, no-one else has reported the problem you had so there is no hurry…take your time.

oxo42 commented 10 years ago

I think I am having this issue with certain devices, but I'm not sure. @nyholku can you send me the test version and I'll check it out

nyholku commented 10 years ago

On 10/09/2014 11:33, John Oxley wrote:

I think I am having this issue with certain devices, but I'm not sure. @nyholku can you send me the test version and I'll check it out


Reply to this email directly or view it on GitHub: https://github.com/nyholku/purejavacomm/issues/47#issuecomment-55085798

I don't know if I can attach files, but here goes, if send direct mail feedback2@sparetimelabs.com, if the attachments don't make it through.

br Kusti

nyholku commented 9 years ago

Closing this for now as I've not heard back from anyone for a while -- feel free to re-open