Fix macOS segfault on close after hardware error #211
Conversation
Just cleaning up the event info struct isn't enough: one some platforms, there's a “drain loop” thread which runs underneath the monitor thread to watch for an empty output buffer. This thread needs to be stopped, too, or it'll segfault and bring down the whole JVM after the event info struct is freed.
The old `MonitorThreadLock` boolean field was only checked at a very slow interval (5s!), and, itself not being synchronized, was prone to race conditions. More importantly, it wasn't set during the monitor thread's self-cleanup after hardware failure, so under typical access patterns, the monitor thread and the application thread would both try to clean up the monitor thread simultaneously. This race condition could occasionally lead to a segfault (only reproduced on macOS, but I've no doubt it could happen elsewhere). I've also attempted to clean up some redundant flag fields, and consolidate setting the remaining fields to further avoid concurrency/reentrancy issues.
|
I can confirm this fixes the behaviour i was seeing on macOS. Thanks! |
|
Great! I have not yet tried this build on any platform other than macOS, nor have I tested behaviour other than failures (have not even run any data through it), so I want to do some more QC before merging. But thank you for trying it so quickly. |
| waitForTheNativeCodeSilly(); | ||
| MonitorThreadAlive=true; | ||
| try { | ||
| this.monitorThreadReady.await(); |
There was a problem hiding this comment.
@MrDOS Any error that occurs during initialise_event_info_struct causes the monitor thread to bail. As the thread dies without ever releasing the lock and pounding the signal, it indefinitely locks the main thread inside this await() call.
We've been testing this PR for a couple of weeks on a monolithic JVM with several webapps and decent network traffic, and keep encountering this hang once in a while. My theory is that due to an inrush burst of filesystem/network activity, the number of file descriptors temporarily exceeds FD_SETSIZE, causing the kernel to hand out fd numbers over that limit, which (rightly) causes initialise_event_info_struct to fail, locking up the whole JVM.
| } catch (InterruptedException e) { | ||
| z.reportln("Interrupted while waiting for the monitor thread to start!"); | ||
| } | ||
| this.monitorThreadState.writeLock().unlock(); |
There was a problem hiding this comment.
This might be a good place to check whether the monitoring thread's initialization phase was successful.
(If it wasn't, throw an exception and clean up the state so the client has a chance to retry.)
This fixes the JVM crash experienced on macOS after a hardware error occurs. The crash was introduced along with the core hardware-error-detection functionality in #172.
In order to generate the
OUTPUT_BUFFER_EMPTYevent, the monitor thread event loop periodically checks the status of the serial port output buffer, just as it checks for other state changes. On platforms which have it, and on Windows where it's emulated, it uses theTIOCSERGETLSRioctl to get the information. On platforms which don't have that ioctl (macOS and FreeBSD <10), the monitor thread starts up another “drain thread” under the monitor thread which loops ontcdrain(3), and generates the event whenever that call unblocks. That separate drain thread is usually terminated byRXTXPort.interruptEventListener(). However, because the monitor thread event loop now internally terminates upon encountering a hardware error, the drain thread cleanup in that interruption method isn't being performed. The fix is to have the event loop call the interruption method when it detects a hardware failure. That ensures all of the normal monitor thread cleanup happens.However, doing that revealed another, rare race condition where the application thread could encounter the port failure and invoke
RXTXPort.close()before the monitor thread began its shutdown (e.g., if the application is sitting on a tight loop aroundavailable(), as in the case of myDisconnectTesttest case). This manifested in another segfault. The obvious solution was some locking around the internal state of the monitor thread. This already kind of existed in the form of theRXTXPort.MonitorThreadLockboolean and theRXTXPort.waitForTheNativeCodeSilly()method which waited for the flag to become unset in a 5-second sleep loop. I took this as an opportunity to replace that nonsense with a real lock. I chose a read/write lock rather than justsynchronizedblocks around an object because it more closely matches the semantics of the old behaviour: all of the I/O functions checked that the flag was clear, which is akin to sharing a read lock, and all of the notification reconfiguration methods set/cleared the flag, which is akin to an exclusive write lock.This should fix the issue @d5smith raised in #197. It probably doesn't fix the original hang-on-
close()/disconnect()issue @mvalla is facing, unless the new locking has accidentally cleaned up another concurrency issue.