No more use of unordered reactions in federated programs #191

arengarajan99 · 2023-04-02T06:39:32Z

The current runtime support for federated execution relies on a set of blocking unordered "control reactions" for each input port that to block the handling of reactions until the required port statuses are known. The drawbacks of this design are discussed in Rengarajan 2023, along with a solution that no longer relies on the use of blocking unordered reactions. This PR implements that solution.

This work was initiated by @arengarajan99 and completed by @petervdonovan.

- Place unsafe reactions onto a hashset, probe hashset as signaled to remove reactions to be safely enqueued if upstream port statuses are known

This reverts commit 318299a.

…ext level

…utput reactions

We are now able to get past startup and print the number 42, but federate 1 hangs after requesting shutdown. Also, this may break things for the other programs.

These are all specific to decentralized coordination.

This also removes two other fields of the reaction_t struct which are not necessary.

After this change I was able to run Python's StopAtShutdown 95 times without observing any deadlock, whereas before I could consistently reproduce the deadlock (when using the macOS runner from CI) by running it several times. It is not entirely clear what this deadlock has to do with the changes in this branch. Although I have not reproduced it in master (I have tried, and after running it 95 times I did not see deadlock), it does seem like a programming mistake to allow the stop tag to increase. Besides, the error was very flaky, non-reproducible on Ubuntu, and quite possibly present even if it is not observed.

So much for failing fast. It seems like this is the behavior that is expected by our tests, so for now maybe it is what we will do.

Thanks to Jacky Kwok for finding this.

core/federated/federate.c

The ability to do this is the initial motivation for this PR.

lhstrh

This is looking very good! Only some comments missing, but other than that, this looks ready to merge...

core/federated/federate.c

Co-authored-by: Marten Lohstroh <[email protected]>

petervdonovan · 2023-07-26T04:56:02Z

I think the timeout in ParallelSourcesMultiport is likely to be a (legitimate) deadlock, although it is hard to be sure since the output was truncated. I do not remember seeing that test deadlock before, so this must be something new. I am trying to reproduce now.

Edit: Failed to reproduce locally on Ubuntu after running 348 times with log level LOG.

Edit: Also failed to reproduce locally on Ubuntu after running 348 times with log level DEBUG.

Edit: Also failed to reproduce on a macOS GH actions runner after running 265 times with log level DEBUG.

petervdonovan · 2023-07-26T20:46:21Z

Could we maybe pretend that everything is fine? I have run ParallelSourcesMultiport hundreds of times locally and in a macOS GH actions runner without being able to reproduce the latest failure; additionally, I have never seen ParallelSourcesMultiport fail when running it on Wessel with random delays inserted.

For the record, this is all the information we got from CI about the apparent deadlock:

Execution output:
    Federate ParallelSourcesMultiport in Federation ID '3da11054f538fe46ef345bbdfe648c7c7e52d705cef6ded8'
    #### Launching the federate federate__s1.
    #### Launching the federate federate__s2.
    #### Launching the federate federate__d1.
    #### Launching the federate federate__t4.
    #### Bringing the RTI back to foreground so it can receive Control-C.
    RTI -i ${FEDERATION_ID} -n 4 -c init exchanges-per-interval 10

It looks like the program never completed startup. It is possible to imagine that it is somehow not a true deadlock (some resource not available on GH's server, etc.) but I cannot say with confidence that it is not a true deadlock.

lhstrh · 2023-07-26T20:51:04Z

Could we maybe pretend that everything is fine? I have run ParallelSourcesMultiport hundreds of times locally and a macOS GH actions runner without being able to reproduce the latest failure; additionally, I have never seen ParallelSourcesMultiport fail when running it on Wessel with random delays inserted.

For the record, this is all the information we got from CI about the apparent deadlock:
Execution output:

    Federate ParallelSourcesMultiport in Federation ID '3da11054f538fe46ef345bbdfe648c7c7e52d705cef6ded8'

    #### Launching the federate federate__s1.

    #### Launching the federate federate__s2.

    #### Launching the federate federate__d1.

    #### Launching the federate federate__t4.

    #### Bringing the RTI back to foreground so it can receive Control-C.

    RTI -i ${FEDERATION_ID} -n 4 -c init exchanges-per-interval 10
It looks like the program never completed startup. It is possible to imagine that it is somehow not a true deadlock (some resource not available on GH's server, etc.) but I cannot say with confidence that it is not a true deadlock.

Sounds fair to me. Should this reappear we'll deal with it then.

Add preliminary support for hashsets in federated runtime

318299a

- Place unsafe reactions onto a hashset, probe hashset as signaled to remove reactions to be safely enqueued if upstream port statuses are known

arengarajan99 added enhancement Enhancement of existing feature help wanted Extra attention is needed federated refactoring labels Apr 2, 2023

arengarajan99 requested review from edwardalee and petervdonovan April 2, 2023 06:39

arengarajan99 assigned lhstrh Apr 2, 2023

arengarajan99 requested a review from lhstrh April 2, 2023 06:39

arengarajan99 and others added 21 commits April 12, 2023 14:44

Revert "Add preliminary support for hashsets in federated runtime"

e05d9ef

This reverts commit 318299a.

Merge branch 'main' into remove-unordered-reactions

06f40da

Added max_level checking to determine if its safe to advance to the n…

f9e54d5

…ext level

Add support for network input w/ STAA decentralized and reorganized o…

c25cd60

…utput reactions

Add additional includes

32dfc3f

Fixes to allow for compilation

a70dd85

Clean up Decentralized code

d7324eb

Add support in decentralized coordination for spawning the STAA thread

366c6d4

Added appropriate locking for STAA thread

3e82ce9

Merge branch 'main' into remove-unordered-reactions

42ae9c2

Temporarily comment out portAbsentReaction.

191a98d

Fix a trivial mistake.

bcdde36

Merge branch 'main' into remove-unordered-reactions

a5a4a19

Stop trace on RTI connection close.

032855f

Possibly make progress on DistributedStop.

0bcec55

We are now able to get past startup and print the number 42, but federate 1 hangs after requesting shutdown. Also, this may break things for the other programs.

Pass DistributedMultiport.

936ff80

Move _lf_start_time_step so it sees current time.

170208f

Get ChainWithDelay to pass locally.

00f28dd

Fix linker error.

13324b5

Pass StopAtShutdown locally.

c1dd61e

Pass DistributedCountPhysical.

0412c1d

petervdonovan force-pushed the remove-unordered-reactions branch from 727fa8e to 687bf89 Compare July 6, 2023 19:33

petervdonovan added 5 commits July 10, 2023 17:24

Delete commented-out code.

5249b27

Add zero-delay action table.

a972384

Merge branch 'main' into remove-unordered-reactions

f85dc17

Fix compilation errors from merge.

afd9bf9

Fix more compilation errors.

fe7270a

These are all specific to decentralized coordination.

petervdonovan changed the title ~~Remove Unordered/Spin Lock Reactions from Federated Runtime~~ Remove Unordered Reactions from Federated Runtime Jul 12, 2023

petervdonovan added 10 commits July 11, 2023 17:25

Address compilation warning/error in GEDF_NP.

e01d85a

Update lingua-franca-ref.txt.

0d4cc07

Fix compilation error in scheduler_adaptive.

0a77ffa

Don't try to invoke STP handler of input reactions

793f350

This also removes two other fields of the reaction_t struct which are not necessary.

Merge branch 'main' into remove-unordered-reactions

9bef1e4

Do not fail on socket write errors.

6a0ab07

So much for failing fast. It seems like this is the behavior that is expected by our tests, so for now maybe it is what we will do.

Fix STP violation that was reported on master.

9428f3d

Thanks to Jacky Kwok for finding this.

Rename output control -> port absent.

f3d62fa

Delete unused code.

b18f360

petervdonovan reviewed Jul 25, 2023

View reviewed changes

core/federated/federate.c Outdated Show resolved Hide resolved

petervdonovan added 2 commits July 25, 2023 12:07

Trivial cleanups.

bd530d5

Remove WORKERS_NEEDED_FOR_FEDERATE.

cfc545b

The ability to do this is the initial motivation for this PR.

petervdonovan marked this pull request as ready for review July 25, 2023 20:36

lhstrh approved these changes Jul 26, 2023

View reviewed changes

core/federated/federate.c Show resolved Hide resolved

core/federated/federate.c Show resolved Hide resolved

core/federated/federate.c Outdated Show resolved Hide resolved

petervdonovan and others added 2 commits July 25, 2023 20:40

Update core/federated/federate.c

3558917

Co-authored-by: Marten Lohstroh <[email protected]>

Add comment for a_port_is_unknown.

64738d1

lhstrh changed the title ~~Remove Unordered Reactions from Federated Runtime~~ No more use of unordered reactions in federated programs Jul 27, 2023

lhstrh mentioned this pull request Jul 27, 2023

No more use of unordered reactions in federated programs; fix for deadlocks in some federated programs lf-lang/lingua-franca#1684

Merged

16 tasks

lhstrh merged commit 8c69304 into main Jul 28, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

No more use of unordered reactions in federated programs #191

No more use of unordered reactions in federated programs #191

arengarajan99 commented Apr 2, 2023 •

edited by lhstrh

Loading

lhstrh left a comment

petervdonovan commented Jul 26, 2023 •

edited

Loading

petervdonovan commented Jul 26, 2023 •

edited

Loading

lhstrh commented Jul 26, 2023 •

edited

Loading

No more use of unordered reactions in federated programs #191

No more use of unordered reactions in federated programs #191

Conversation

arengarajan99 commented Apr 2, 2023 • edited by lhstrh Loading

lhstrh left a comment

Choose a reason for hiding this comment

petervdonovan commented Jul 26, 2023 • edited Loading

petervdonovan commented Jul 26, 2023 • edited Loading

lhstrh commented Jul 26, 2023 • edited Loading

arengarajan99 commented Apr 2, 2023 •

edited by lhstrh

Loading

petervdonovan commented Jul 26, 2023 •

edited

Loading

petervdonovan commented Jul 26, 2023 •

edited

Loading

lhstrh commented Jul 26, 2023 •

edited

Loading