Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixes double open() call on unix #670

Closed
wants to merge 1 commit into from

Conversation

gfcittolin
Copy link
Contributor

Fixes a double call to open() on unix, that leaks a file descriptor and keeps the underlying device open, as discussed on #661

@shoerob
Copy link

shoerob commented Feb 23, 2016

I'm encountering a problem on OS X that might be related to this. When I open() my device, the data comes through just fine. But if I close() it, then open() it again, no data comes through anymore. I have to unplug it, and plug it back in again in order to open() and receive data again. The strange thing is that open() doesn't complain when this is happening, just no data comes through.

Update: Finally got a chance to try this, and it fixed the problem. Thanks!

@JonathonAshworth
Copy link

I'm having the exact same problem as shoerob, managed to find it here through search but there doesn't seem to be an open issue. Is this merge really a fix for it? Seems unrelated... but hey, whatever works!

How long until this is accepted?

@reconbot
Copy link
Member

reconbot commented Mar 2, 2016

@JonathonAshworth have you not found any open issues about this?

I'll run some tests in the next day or two. Should be able to work this into the next release, I can clearly see there's a problem, but I need to put it through its paces and reproduce @shoerob's issue.

@limbera
Copy link

limbera commented Mar 2, 2016

I am experiencing a similar issue to @shoerob but this fork did not solve it for me.

@reconbot
Copy link
Member

reconbot commented Mar 2, 2016

Can you give me some code and more information so I can reproduce it?

On Wed, Mar 2, 2016, 6:09 PM Andreas Limberopoulos [email protected]
wrote:

I am experiencing a similar issue to @shoerob https://github.com/shoerob
but this fork did not solve it for me.


Reply to this email directly or view it on GitHub
#670 (comment)
.

@JonathonAshworth
Copy link

The relevant code is quite chunky and we're having trouble narrowing down the problem. I've put it in a gist, but I'm going to try and come up with a minimum representation of the problem now so you've got something a bit cleaner to work with!

https://gist.github.com/JonathonAshworth/a099183982a8c40747ab

Essentially, our code opens each serial port on the computer in turn, sends a test packet to verify that the device responds to the MTK format, then requests firmware information from the device to verify that it's a valid device that was bought from our company. Then it closes the port.

This happens for every valid serial port on the computer, generating an array of "valid ports" (i.e ports which represent one of our devices). Then those valid ports are opened a second time so data can be downloaded off them. On the second open, a test packet is once again sent, then a packet requesting the memory status of the device is sent, followed by downloading the actual memory off the device.

The problem appears to be with the open-close-open process, and happens only on OSX. If we do not check the validity of a device first, then it downloads just fine, but if the device has been opened and closed previously, then it will be successfully reopened, but will fail later on. Half the time it fails to respond to the test packet, and half the time it successfully responds to the test packet, then fails to respond to the memory status request.

The curious part to me at least is that it's sometimes successfully responding to the test packet before failing. It seems as if the device is being randomly closed or randomly failing.

@JonathonAshworth
Copy link

https://gist.github.com/JonathonAshworth/ecafc8a2df376a4063e8

Alright, this is the simplest test I could come up with that is failing. The device constantly outputs a stream of realtime gps data, so the test just opens the device for 5 seconds, logging it's output to console, closes it, then re-opens it for another 5 seconds.

This acts as you'd expect on both Ubuntu and Windows, but on Mac, it only ever outputs the very first time the device is opened after plugging it in. The second time it's opened nothing prints, and on subsequent runs it doesn't print either time, until the device is unplugged and plugged in again.

My workmate just installed this fork and is reporting that it's working as expected. So it appears this is (somehow) a fix for the issue.

@gfcittolin
Copy link
Contributor Author

gfcittolin commented Mar 3, 2016 via email

@@ -131,6 +131,9 @@ void EIO_Open(uv_work_t* req) {
OpenBaton* data = static_cast<OpenBaton*>(req->data);

int flags = (O_RDWR | O_NOCTTY | O_NONBLOCK | O_CLOEXEC | O_SYNC);
if(data->hupcl == false) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a good patch and I'll incorporate it into my fork but this block should be removed, not moved. HUPCL is not an fcntl flag; it's a tty control flag and has nothing to do with open(2). On Linux x86_64 HUPCL happens to have the same value as O_APPEND so is benign but on other platforms this code might change the intended open behavior.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually it should be moved within the setup() function to be something like this. (Cherry-pick that if desired.)

reconbot added a commit that referenced this pull request Apr 4, 2016
 - includes #670 from @gfcittolin
 - includes patch from @pabigot moving when we apply HUPCL
 - Close the fd if we have any errors during setup
 - Properly receive errors when setting a custom baud rate on OS X
reconbot added a commit that referenced this pull request Apr 4, 2016
 - includes #670 from @gfcittolin
 - includes patch from @pabigot moving when we apply HUPCL
 - Close the fd if we have any errors during setup
 - Properly receive errors when setting a custom baud rate on OS X
@reconbot
Copy link
Member

reconbot commented Apr 4, 2016

I've incorporated this and a few more fixes into #728

Thank you!

@reconbot reconbot closed this Apr 4, 2016
@lock lock bot locked as resolved and limited conversation to collaborators Jan 18, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

6 participants