Improve the usage of Sysfs in the interaction with EV3Dev

[jabrena]

#765

[JakubVanek]

Oh, nice 😄

[jabrena]

hi @dwalend, In relation to the point:

I think there are two reasonable approaches: synchronized read() and close() or let the caller do the work.

With current implementation, I don´t see issues for concurrent reads, so my question, do you see something pending at Battery level?

[JakubVanek]

@dwalend

I guess my big opening question before examining the test code is: how do you want this to behave concurrently? I think there are two reasonable approaches: synchronized read() and close() or let the caller do the work. I'm fine with either. I'm not sure of the value of writing tests of concurrent behavior until we define that expected behavior.

I too think both can be considered safe in their respective assumption sets. The issue I see with the non-synchronized approach is that it breaks code that previously worked/had different assumptions (and that might be OK). There is also another issue - Battery is made a thread-unsafe singleton by this change. If it wasn't a singleton, I could create a per-thread instance and the race would be gone (mitigating on the user side).

Also, synchronized may not be necessary, DataChannelRereader2 in #775 seems not to need them.

The follow-up is: how would you like to handle times when the JVM catches the OS midway through writing a file while the JVM tries to read it? Returning an empty string from readString() seems reasonable in a very local sense, but will still blow up.

I think we cannot handle this differently (empty attribute is a valid attribute). Also, if the user wants to read an integer from an empty file, the code has to blow up, doing something different would hamper debugging in my opinion.

However, this reader/writer race shouldn't be an issue for real sysfs in /sys - the window where the file is invalid shouldn't exist there. If it is, then it was malformed code even now and I don't see much value in covering that case.

Shouldn't it return zero for an empty file, different from -1 , end of stream? I'll do that experiment next.

I thought the same, but stepping through the code in IDE and seeing the file as really empty changed my mind.

[JakubVanek]

I'm not sure about synchronizing DataChannelRewriter. As we now know that concurrent reads/writes are problematic, I'd probably be OK with letting that break.

[JakubVanek]

@jabrena I added a test for the race that could happen in Battery: https://github.com/ev3dev-lang-java/ev3dev-lang-java/pull/775/files#diff-6a3ca90b6a8e8b1ea64f7fe17c393c8d8040b33d419856eb6fb2296c9ca7376d

[dwalend]

Oh - you can't presuppose ThreadLocals will be OK. They are an allocation disaster with thread pools on a large scale. Using them forces a possibly-inappropriate system-level decision on everyone using the library. Probably not a big problem for EV3, but it forces a single solution. In particular I won't be able to use cats IO.

One sane way to use these things multithreaded is likely to read from the OS and update a volatile value in the JVM with a task. If that thread is from a pool then ThreadLocal starts allocating per thread, out of the developer's control.

[JakubVanek]

I was a bit worried about it (mentioned in javadoc). It then seems that ThreadLocals are KO.

I see the following remaining options with regards to Battery:

• accept that the Battery singleton needs external synchronization (if to be accessed concurrently)
• make Battery not a singleton
• synchronize DataChannelReader (may have locking overhead)
• allocate a new buffer each time a read is made (may increase allocation/GC overhead)

[jabrena]

hi @dwalend,

what other alternatives do, we have apart of ThreadLocals for multiple concurrent Readers in your humble opinion?

[jabrena]

@JakubVanek,
we are reviewing a general way to interact with the ev3dev filesystem, not an specific solution for one Sensor.

[JakubVanek]

@JakubVanek,
we are reviewing a general way to interact with the ev3dev filesystem, not an specific solution for one Sensor.

It does partially generalize, I think. If we solve this for Battery, other devices should follow easily. For classes where constructors are public (/non-singletons), thread ownership could be additionally modelled by hiding the reference to the device instance from other threads (i.e. how DataChannelRereader worked in old DataChannelRereaderConcurrencyTest). For singletons, all threads have access to one common reference, a situation that seems harder to solve.

[dwalend]

We could do an internal lock on read() and close() instead of synchronized on the two methods. That offers some protection vs. a diabolical problem - someone else using DataChannelRereader as their own lock - that never happens.

Another alternative is to allocate something new for every read - Sysfs was already doing that.

We could set up our own reading thread, and put whatever we read in a volatile. That seems heavy-handed, premature optimization.

Most everything else is systemic and we should avoid doing it in a library.

[jabrena]

Hi @dwalend

It is better, if we try with this alternative:

We could do an internal lock on read() and close() instead of synchronized on the two methods. That offers some protection vs. a diabolical problem - someone else using DataChannelRereader as their own lock - that never happens.

The another alternative increase the complexity.

Lets try with the locks. :)

[JakubVanek]

We could set up our own reading thread, and put whatever we read in a volatile. That seems heavy-handed, premature optimization.

While my opinion is not based on numbers, in some cases this might end up slower than before. Doing this in a separate thread would add periodic context switches (these might happen due to JIT/GC too though) and it could make the data out-of-date (the thread is likely not going to be scheduled each millisecond).

[dwalend]

synchronized is a fine answer, especially if we can't measure a performance hit after JIT and hotspot kick in. Hotspot should be able to optimize away the lock.

[dwalend]

Details were easy to find, but are maybe a bit dated - 2008: https://wiki.openjdk.java.net/display/HotSpot/Synchronization

"Only the first lock acquisition performs an atomic compare-and-swap to install an ID of the locking thread into the header word. The object is then said to be biased towards the thread. Future locking and unlocking of the object by the same thread do not require any atomic operation or an update of the header word. Even the lock record on the stack is left uninitialized as it will never be examined for a biased object."

[dwalend]

Ev3LedTest is a bit broken; it takes advantage of Sysfs's

    public static boolean writeString(final String filePath, final String value) {
            if (file.canWrite()) {
                //

evaluating to false and, well, not really testing anything.
I'm going to skip tests for now to get the code to where I can show it.

[dwalend]

#781 in for preliminary review.

[dwalend]

And I updated #778

[jabrena]

Today, I will be off. Family matters