Oh, nice 😄

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?

@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.

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

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.

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.

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

publicstaticbooleanwriteString(finalStringfilePath,finalStringvalue) {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.

#781 in for preliminary review.

And I updated#778

Today, I will be off. Family matters