DriverStation.isNewControlData and DriverStation.waitForData guarantees. about allwpilib

[ThadHouse]

Looking at the code again, with #187 in place, the IsNewControlData variable will always be atomically set right before waitForData is woken up. I don't really see much of a use case for needing to check for both waitForData and isNewControlData in the same loop however. But if you do, isNewControlData will always be set to true before waitForData returns.

Regarding the InterruptedException, for that to happen the user code would have to interrupt their own thread that is waiting for new data, which is something that is highly frowned upon doing in current user code. If somebody interrupts a thread, they've done it on purpose, and I don't think we can keep the guarantee on waitForData at that point.

One note is that changes made to block spurious wake ups have made it so you can only call waitForData from 1 thread. If you call it from multiple threads only one thread will be woken up.

[virtuald]

If we want to allow the user to use interrupts, then it should throw the interrupt. This usage doesn't make sense.

[ThadHouse]

If we want the users to be able to interrupt the thread, then I do not believe it is possible to hold these guarantees in the case of the thread being interrupted. Also, because of the way waitForData is used in the IterativeRobot base class, it is actually impossible for a user to handle the thread interrupted exception anyway unless they write their own base class. We do reset the interrupted flag, so if users want to check for an interrupt they can, but they should know how to do that properly if they are ever thinking about interrupting their main robot thread and creating their own base class to do so.

[virtuald]

That's a good point. If we want to preserve the guarantee (important!) and allow the user to deal with interrupts, perhaps the following approach would be better?

public void waitForData(long timeout) {
  synchronized (m_dataSem) {
    while (!m_updatedControlLoopData) {
      try {
        m_dataSem.wait(timeout);
      } catch (InterruptedException ex) {
        if (onInterrupt(e))
          return;
      }
    }
    m_updatedControlLoopData = false;
  }
}

// add a note here for advanced users only
public boolean onInterrupt(InterruptedException e) {
  return false;
}

[ThadHouse]

How would onInterrupt work? Would we be expecting users to override it? Because there is no current way to override DriverStation easily. To do this that way, we would probably be better of passing in a functional interface that would then be called in onInterrupt(). Unless I'm confused on how this would be any different.

However, I still don't know if preserving the guarantee on interrupts is a necessary requirement. In all other cases yes it should be, but throwing a thread interrupt on the main user thread is 99.9% of the time just going to end the program anyway, at which point the guarantee there is moot. Only 1 thread can call waitForData() anyway, which for any team that doesn't create their own base class is the base classes we provide which couldn't catch the exception anyway.

[virtuald]

Oh.. ha, for some reason I was thinking that would be in iterative robot. Never mind. Hm.

[virtuald]

Well, consider this:

• Someone causes Thread.interrupt to happen
• Wait for data returns immediately
• User code neglects to check for interrupt for whatever reason
• isNewData returns false, so IterativeRobot periodic code doesn't execute
• Wait for data returns immediately because thread.interrupt is set
• And so on...

[ThadHouse]

I think we should remove the isNewControlData calls from iterative robot. If you move the waitForData to the top of the loop the behavior doesn't change. And then in waitForData reset the flag before wiating. If the user didn't handle it in the loop then ran their code isn't going to do anything anyway

[virtuald]

If you reset the flag before waiting that introduces a race condition.

[ThadHouse]

How does that race, and with what?

[virtuald]

• Thread 1: user periodic happens
• Thread 1: waitForData is called
• Thread 2: thread1.interrupt()
• Thread 1: waitForData resets interrupt flag

[ThadHouse]

Well no matter if you reset it in user periodic or wait for data that race happens. Isn't that why thread interrupts are deprecated and highly frowned upon in most cases, since its almost impossible to get rid of all race conditions

[virtuald]

Thread.stop is deprecated, Thread.interrupt is not deprecated.

The difference is that if you reset it in user periodic you're able to take some action based on the state of the flag. If waitForData just discards it, you have no way of knowing that it happened AND you have to wait the 20ms or so until you're able to find out about the condition happening.

Discarding the thread interrupt unintentionally is a bad thing. Intentionally ignoring thread interrupt is fine, as long as you're intentionally doing it.

[ThadHouse]

I think if a user doesn't handle it in their periodic code, we want to intentionally ignore it.

[virtuald]

Then it needs to do so explicitly:

while (!m_updatedControlLoopData) {
   try {
     m_dataSem.wait(timeout);
  } catch (InterruptedException e) {
     // ignored
  }
}

Which... looking at the code, made me realize that the timeout is ignored too. :(

[virtuald]

I think you have to either propagate the exception, or always ignore the interrupt -- sometimes allowing the interrupt to work is inconsistent and in my opinion a bad thing.

[ThadHouse]

Yeah I think we should just swallow the interrupt completely. No way for user code to then see it, but that really doesn't matter. Users don't really have a reason to interrupt the main thread.

That's a good point about ignoring the timeout. However upon a timeout those guarantees would be gone as well, but the only way for a timeout case to happen is a custom base class, which I don't think we can hold the guarantees anyway as those base classes can work around that in a different way.

[ThadHouse]

Propagating the exception would only work with custom base classes as well. The WPILib provided base classes could not handle them in user code.

[virtuald]

The timeout could be built into the guarantee, just change the API to return false if the data isn't present yet.

Synchronization like this is hard. I really like the guava Monitor/Monitor.Guard classes for this sort of thing, instead of trying to roll it myself.

[ThadHouse]

Yeah we had a discussion a few months ago on the predicate for stopping spurious wakeups. So yeah we should change waitForData to return the timeout and swallow the interrupt. I'll do that.

[virtuald]

That would be reverting #24, see also #12?

[ThadHouse]

Uhhh. Java's wait doesn't return anything... How do we check for a timeout then? We can't check isNewControlData as we have a race then, if a separate user thread calls isNewControlData between the wait returning and checking the flag.

[virtuald]

Yeah the timeout thing sucks. Looks like guava uses lock objects instead of raw java objects: https://github.com/google/guava/blob/master/guava/src/com/google/common/util/concurrent/Monitor.java#L383

[ThadHouse]

Yeah we might have to rewrite that using separate objects in order to get it working properly.

[AustinShalit]

Closed with #252?

[PeterJohnson]

Yes, closed with #252.