Need to reinstate send suspend logic

[jpaulm]

Need to reinstate send suspend logic - this seems to be mostly in PR #74

[dancrumb]

I want to make sure I understand the logic that you're talking about.

This is the logic that suspends a receiver when there are no IPs currently in any incoming connections to the InputPort that the receiver has called receive on. Is that correct?

[dancrumb]

On further investigation, I see that the whole receive code is broken, because there's no support for multiple connections coming into an InputPort.

If you have the patience, I'd like to spend some time working on resolving this. It will probably take a few days. I'll work on a branch so that there is a reviewable Pull Request.

If you're already working on a fix, just let me know.

[jpaulm]

Re the post before your last one: what seems to have disappeared is the logic to suspend a sender when the connection being sent to is full. If I'm right, this would explain why the app you were working on saturates storage. I referred you to the discussion of ballooning in the book - this is not a desirable behaviour, as logic errors can behave like storage errors, and the application behaviour becomes much less predictable.

In this case you can look at the code before PR #74 was merged in to see how connections and the related ports are interrelated.

[jpaulm]

Re your last post, I am happy to let you do this. I have reset my personal version of jsfbp to the one before PR #74 was merged in. My initial test worked fine, and I could happily run with this version. However it is rather a sledge-hammer approach, and would cause jsfbp to lose some doubtless useful changes that you put in later. So if you can be more surgical about removing the effects of PR #74 on the connection logic, that would be much better.

However, I want to say that I really dislike people refactoring my code, and moving stuff around, as it makes it nearly impossible for me to see what has changed. Sorry if this offends your esthetic sense :-) , but my classes are chosen rather carefully, and I don't like to see them changed in any significant way. In this case your refactoring seems to have led to quite a bit of confusion - on both our parts!

See also my earlier comment about how the Travis test didn't pick up on this at all, leading me to think everything was fine... when it wasn't!

You are actually the 3rd person to try refactoring various implementations - first the JavaFBP implementation some years ago, then the C#FBP one, and now jsfbp. In both of the previous cases, it became obvious that the people involved were confused at a very fundamental level, so we had to give up on the effort!

Hope we can continue to work together - there is a lot of good stuff here!

[jpaulm]

Hi Dan,

How is this going? I realized I should have posted a warning on the Readme
when I found about this. GitHub seems to be slow right now, but I will do
it as soon as possible - unless I hear from you differently.

Regards,

Paul

On Fri, Nov 11, 2016 at 4:51 PM, Dan Rumney [email protected]
wrote:

On further investigation, I see that the whole receive code is broken,
because there's no support for multiple connections coming into an
InputPort.

If you have the patience, I'd like to spend some time working on resolving
this. It will probably take a few days. I'll work on a branch so that there
is a reviewable Pull Request.

If you're already working on a fix, just let me know.

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#78 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AATGJ3sJ_rpKPDNHUwFOReUpNibK4mdLks5q9ONagaJpZM4KvycM
.

[jpaulm]

PS In my npm test breader seems to have broken - can I fix it, or drop it
from npm test? If the latter, where are the tests stored?

Thx

Paul

On Wed, Nov 16, 2016 at 9:54 AM, Paul Morrison [email protected] wrote:

Hi Dan,

How is this going? I realized I should have posted a warning on the
Readme when I found about this. GitHub seems to be slow right now, but I
will do it as soon as possible - unless I hear from you differently.

Regards,

Paul

On Fri, Nov 11, 2016 at 4:51 PM, Dan Rumney [email protected]
wrote:

On further investigation, I see that the whole receive code is broken,
because there's no support for multiple connections coming into an
InputPort.

If you have the patience, I'd like to spend some time working on
resolving this. It will probably take a few days. I'll work on a branch so
that there is a reviewable Pull Request.

If you're already working on a fix, just let me know.

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#78 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AATGJ3sJ_rpKPDNHUwFOReUpNibK4mdLks5q9ONagaJpZM4KvycM
.

[dancrumb]

Hi Paul,

Went away for the weekend and been catching up.

Regarding suspending the sender, I believe this remains in place in the ProcessConnection code, specifically at: https://github.com/jpaulm/jsfbp/blob/master/core/ProcessConnection.js#L79

At this point, if the number of pending IPs is greater than the connection capacity, then the process is put into WAITING_TO_SEND status and is not requeued for the runtime; then, the process yields, thereby suspending it until it's added to the runtime queue again.

This check occurs in a permanent loop that isn't broken until the IP that is being 'sent' can be added to the connection.

The absence of tests around this behaviour is definitely an oversight and a problem. Perhaps the first step is to try and craft a test that passes when the behaviour is correct and otherwise fails.

Certainly Travis didn't catch this, but I think that's more a shortcoming of the test suite rather than an inherent shortcoming of CI.

I think an update to the README is probably a good idea. Alternatively, another approach might be to branch off the current master branch and then revert all of the changes I put in, with a view to being more circumspect about the changes.

Finally, you can modify https://github.com/jpaulm/jsfbp/blob/master/test/components/breader.js#L9 to read describe.skip to skip that test case. I think it's better to 'skip' than remove, since you have that reminder that you have a test that's not currently working without it breaking your workflow.

[dancrumb]

As I look at it, you should roll my work back... my implementation of connections is way off.

My ProcessConnection class has a single capacity, but multiple upstream processes... this is a totally incorrect representation of the way that multiple OutputPorts can fan into an InputPort.

[jpaulm]

Thanks, Dan, it would be best for now. A few days ago I did a git reset
--hard to the commit before your big merge, and npm test works, except for
breader.

When I do a git status, I get:

C:\Users\Paul\Documents\GitHub\jsfbp [master ↓]> git status
On branch master
Your branch is behind 'origin/master' by 28 commits, and can be
fast-forwarded.
(use "git pull" to update your local branch)
nothing to commit, working tree clean

What is the correct git statement to set the repo to this level for the
time being?

Thanks,

Paul

On Wed, Nov 16, 2016 at 12:02 PM, Dan Rumney [email protected]
wrote:

As I look at it, you should roll my work back... my implementation of
connections is way off.

My ProcessConnection class has a single capacity, but multiple upstream
processes... this is a totally incorrect representation of the way that
multiple OutputPorts can fan into an InputPort.

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#78 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AATGJ8v7vC3l-JOe3GMoUlNUP2a5p-FXks5q-zc0gaJpZM4KvycM
.

[jpaulm]

No that's correct - the ProcessConnection has the capacity, as it is where
in transit IPs are held. OutputPorts only have a "closed" attribute. See
https://github.com/jpaulm/javafbp/blob/master/src/main/java/com/jpaulmorrison/fbp/core/engine/OutputPort.java

On Wed, Nov 16, 2016 at 12:02 PM, Dan Rumney [email protected]
wrote:

As I look at it, you should roll my work back... my implementation of
connections is way off.

My ProcessConnection class has a single capacity, but multiple upstream
processes... this is a totally incorrect representation of the way that
multiple OutputPorts can fan into an InputPort.

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#78 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AATGJ8v7vC3l-JOe3GMoUlNUP2a5p-FXks5q-zc0gaJpZM4KvycM
.

[dancrumb]

OK... so here's what to do:

git reset --soft <commit hash>
git add .
git commit -m "Rollback breaking changes"
git push

That will create a new commit that rolls everything back, rather than destroying the shared history. That way, the changes are still viewable, but they're no longer at the front of the tree

[dancrumb]

Yes... the ProcessConnection should have capacity, but it should be between a single upstream process and a single downstream process. My implementation has it between multiple upstream processes and a single downstream process. This is incorrect, since, in the case where multiple OUT ports lead into an IN, there could be a different capacity for each connection in the FBP graph.

[dancrumb]

An example would be:

aplha OUT -> (5) IN gamma
beta OUT -> (10) IN gamma
delta OUT -> IN gamma

[dancrumb]

Also, the OutputPort does have a closed property; it's inherited from the Port class

[jpaulm]

In this case, jsfbp will differ from all the other implementations! We
have always allowed multiple output ports (on different processes) to feed
into a single Connection, but in-transit IPs are held in the Connection,
not in the output ports - the IPs arriving at that connection are merged in
a first-come, first-served order. If you don't do that, how would you
decide in what order to read from the multiple outports... if you could,
which you can't, as "receive" references a connection, not an output port.

Sorry, Dan!

On Wed, Nov 16, 2016 at 12:35 PM, Dan Rumney [email protected]
wrote:

Yes... the ProcessConnection should have capacity, but it should be
between a single upstream process and a single downstream process. My
implementation has it between multiple upstream processes and a single
downstream process. This is incorrect, since, in the case where multiple
OUT ports lead into an IN, there could be a different capacity for each
connection in the FBP graph.

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#78 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AATGJ4iQkcbFkHBzYbM27PnJqWBg6e4gks5q-z77gaJpZM4KvycM
.

[jpaulm]

"Finally, you can modify https://github.com/jpaulm/jsfbp/blob/master/test/
components/breader.js#L9 to read describe.skip to skip that test case. I
think it's better to 'skip' than remove, since you have that reminder that
you have a test that's not currently working without it breaking your
workflow."

Do you leave the function, or remove the whole thing?

TIA

Paul

On Wed, Nov 16, 2016 at 12:00 PM, Dan Rumney [email protected]
wrote:

Hi Paul,

Went away for the weekend and been catching up.

Regarding suspending the sender, I believe this remains in place in the
ProcessConnection code, specifically at: https://github.com/jpaulm/
jsfbp/blob/master/core/ProcessConnection.js#L79

At this point, if the number of pending IPs is greater than the connection
capacity, then the process is put into WAITING_TO_SEND status and is not
requeued for the runtime; then, the process yields, thereby suspending it
until it's added to the runtime queue again.

This check occurs in a permanent loop that isn't broken until the IP that
is being 'sent' can be added to the connection.

The absence of tests around this behaviour is definitely an oversight and
a problem. Perhaps the first step is to try and craft a test that passes
when the behaviour is correct and otherwise fails.

Certainly Travis didn't catch this, but I think that's more a shortcoming
of the test suite rather than an inherent shortcoming of CI.

I think an update to the README is probably a good idea. Alternatively,
another approach might be to branch off the current master branch and then
revert all of the changes I put in, with a view to being more circumspect
about the changes.

Finally, you can modify https://github.com/jpaulm/jsfbp/blob/master/test/
components/breader.js#L9 to read describe.skip to skip that test case. I
think it's better to 'skip' than remove, since you have that reminder that
you have a test that's not currently working without it breaking your
workflow.

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#78 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AATGJxPqLAWGFARu3U3GLSnowyOAedGTks5q-zangaJpZM4KvycM
.

[dancrumb]

So, in the case of the graph above, having a single capacity for a Connection object doesn't make sense, if that connection encapsulates all incoming connections to an IN port.

I can see how an implementation of a Connection could do this, but not with a single capacity value... there would need to be one for each incoming processes. Definitely doable, but not what appears to be in place in the Java implementation.

[dancrumb]

For skipping a test:

describe( /* everything */);

becomes

describe.skip( /* everything */);

[jpaulm]

B**dy GitHub!

I got to git push - but I had updated Readme, so it said to do a pull
first. Did that, and got:

remote: Counting objects: 3, done.
remote: Compressing objects: 100% (3/3), done.
remote: Total 3 (delta 0), reused 0 (delta 0), pack-reused 0
Unpacking objects: 100% (3/3), done.
From https://github.com/jpaulm/jsfbp
8432d81..89ae5d7 master -> origin/master
Auto-merging core/trace.js
CONFLICT (content): Merge conflict in core/trace.js
Auto-merging core/runtimes/FiberRuntime/index.js
CONFLICT (content): Merge conflict in core/runtimes/FiberRuntime/index.js
Auto-merging core/ProcessConnection.js
CONFLICT (content): Merge conflict in core/ProcessConnection.js
Auto-merging core/Process.js
CONFLICT (content): Merge conflict in core/Process.js
Auto-merging core/OutputPort.js
CONFLICT (content): Merge conflict in core/OutputPort.js
Auto-merging core/Network.js
CONFLICT (content): Merge conflict in core/Network.js
Auto-merging core/InputPort.js
CONFLICT (content): Merge conflict in core/InputPort.js
Auto-merging core/IIPConnection.js
CONFLICT (content): Merge conflict in core/IIPConnection.js
CONFLICT (modify/delete): core/FIFO.js deleted in HEAD and modified in
89ae5d7
08faef88399b415e42fdb8dd8205ca5. Version
89ae5d7
5 of core/FIFO.js left in tree.
CONFLICT (modify/delete): core/Connection.js deleted in HEAD and modified
in 89a
e5d79208faef88399b415e42fdb8dd8205ca5. Version
89ae5d7
8205ca5 of core/Connection.js left in tree.
Auto-merging components/repl.js
Auto-merging README.md
Automatic merge failed; fix conflicts and then commit the result.
C:\Users\Paul\Documents\GitHub\jsfbp [master ↕ +2 ~16 -2 !8 | +0 ~0 -0 !10
!]>

Try again?! Maybe if I did a reset --hard from the same commit hash...?
Would that get rid of merge indicator?

(Sorry!)

Paul

On Wed, Nov 16, 2016 at 12:34 PM, Dan Rumney [email protected]
wrote:

OK... so here's what to do:

git reset --soft
git add .
git commit -m "Rollback breaking changes"
git push

That will create a new commit that rolls everything back, rather than
destroying the shared history. That way, the changes are still viewable,
but they're no longer at the front of the tree

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#78 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AATGJ5IveEu51o8h-x6FNKOWfwZ_NlEyks5q-z6fgaJpZM4KvycM
.

[dancrumb]

Git can be a pain when you're trying revert stuff :)

OK... so, first, make sure you have no changes that have not been committed (shouldn't be a problem).

1. Reset your local repo to match the remote one:

git fetch origin
git reset --hard origin/master

2. Soft reset to the commit you want to be at:

git reset --soft <commit hash>

3. Create a new commit to rollback to this point in time:

git add .
git commit -m "Rollback breaking changes"

4. Push the new state to Github:

git push

[jpaulm]

Close but no banana!

The final git push said

C:\Users\Paul\Documents\GitHub\jsfbp [master ↕]> git push
To https://github.com/jpaulm/jsfbp.git
! [rejected] master -> master (non-fast-forward)
error: failed to push some refs to 'https://github.com/jpaulm/jsfbp.git'
hint: Updates were rejected because the tip of your current branch is behind
hint: its remote counterpart. Integrate the remote changes (e.g.
hint: 'git pull ...') before pushing again.
hint: See the 'Note about fast-forwards' in 'git push --help' for details.

Sorry, have to run out for a couple of hours - I feel we're nearly there!

On Wed, Nov 16, 2016 at 1:10 PM, Dan Rumney [email protected]
wrote:

Git can be a pain when you're trying revert stuff :)

OK... so, first, make sure you have no changes that have not been
committed (shouldn't be a problem).

1. Reset your local repo to match the remote one:

git fetch origin
git reset --hard origin/master

1. Soft reset to the commit you want to be at:

git reset --soft

1. Create a new commit to rollback to this point in time:

git add .
git commit -m "Rollback breaking changes"

1. Push the new state to Github:

git push

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#78 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AATGJx-Fw1mwtiR0eIpL6wp-JQXg8GgNks5q-0cigaJpZM4KvycM
.

[dancrumb]

OK finally figured it out:

git reset --hard <your commit of choice>
git clean -df
git reset --soft @{1}
git commit -m "Rollback of breaking changes"
git push

I did this on my copy, ran npm install and npm test and everything (except breader) seemed to work.

The breader issue is a line-endings issues due to OS differences.

[jpaulm]

Thanks, Dan,

Will give that a try as soon as I get home!

Regards,

Paul

On Nov 16, 2016 2:06 PM, "Dan Rumney" [email protected] wrote:

OK finally figured it out:

git reset --hard
git clean -df
git reset --soft @{1}
git commit -m "Rollback of breaking changes"
git push

I did this on my copy, ran npm install and npm test and everything
(except breader) seemed to work.

The breader issue is a line-endings issues due to OS differences.

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#78 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AATGJ4zO3efM4534-rzzio_IFH6vL8beks5q-1RMgaJpZM4KvycM
.

[jpaulm]

Never saw git clean before! Interesting!

No errors...

Did next statement, and got

C:\Users\Paul\Documents\GitHub\jsfbp [master ↓]> git reset --soft @{1}
At line:1 char:21

• git reset --soft @{1}

•                 ~
  

  Missing '=' operator after key in hash literal.
  At line:1 char:21
• git reset --soft @{1}

•                 ~
  

  The hash literal was incomplete.
  • CategoryInfo : ParserError: (:) [],
    ParentContainsErrorRecordEx
    ception
  • FullyQualifiedErrorId : MissingEqualsInHashLiteral

Huh?! Maybe my posh~git is different from yours...?

On Wed, Nov 16, 2016 at 2:06 PM, Dan Rumney [email protected]
wrote:

OK finally figured it out:

git reset --hard
git clean -df
git reset --soft @{1}
git commit -m "Rollback of breaking changes"
git push

I did this on my copy, ran npm install and npm test and everything
(except breader) seemed to work.

The breader issue is a line-endings issues due to OS differences.

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#78 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AATGJ4zO3efM4534-rzzio_IFH6vL8beks5q-1RMgaJpZM4KvycM
.

[dancrumb]

Try ORIG_HEAD instead of @{1}

It could be that '@{1}' has some special meaning to your shell

On Wed, Nov 16, 2016 at 3:31 PM Paul Morrison [email protected]
wrote:

Never saw git clean before! Interesting!

No errors...

Did next statement, and got

C:\Users\Paul\Documents\GitHub\jsfbp [master ↓]> git reset --soft @{1}
At line:1 char:21

• git reset --soft @{1}
• ~
  Missing '=' operator after key in hash literal.
  At line:1 char:21
• git reset --soft @{1}
• ~
  The hash literal was incomplete.
• CategoryInfo : ParserError: (:) [],
  ParentContainsErrorRecordEx
  ception
• FullyQualifiedErrorId : MissingEqualsInHashLiteral

Huh?! Maybe my posh~git is different from yours...?

On Wed, Nov 16, 2016 at 2:06 PM, Dan Rumney [email protected]
wrote:

OK finally figured it out:

git reset --hard
git clean -df
git reset --soft @{1}
git commit -m "Rollback of breaking changes"
git push

I did this on my copy, ran npm install and npm test and everything
(except breader) seemed to work.

The breader issue is a line-endings issues due to OS differences.

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#78 (comment), or
mute
the thread
<
https://github.com/notifications/unsubscribe-auth/AATGJ4zO3efM4534-rzzio_IFH6vL8beks5q-1RMgaJpZM4KvycM

.

—
You are receiving this because you commented.

Reply to this email directly, view it on GitHub
#78 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AANhT7bB1ltGJoMcF-eOR9CfCO3IJgeKks5q-3YugaJpZM4KvycM
.

[jpaulm]

Dan, please ignore the previous note - I tried it in regular command
prompt, and it worked... until the git push, which gave the same messages
again as before:

error: failed to push some refs to 'https://github.com/jpaulm/jsfbp.git'
hint: Updates were rejected because the tip of your current branch is behind
hint: its remote counterpart. Integrate the remote changes (e.g.
hint: 'git pull ...') before pushing again.
hint: See the 'Note about fast-forwards' in 'git push --help' for details.

I assume this is because of the change I made to Readme - if so, my
fault... but it should be possible to get around it, as I don't case if my
recent change to Readme gets wiped.

On Wed, Nov 16, 2016 at 4:31 PM, Paul Morrison [email protected] wrote:

Never saw git clean before! Interesting!

No errors...

Did next statement, and got

C:\Users\Paul\Documents\GitHub\jsfbp [master ↓]> git reset --soft @{1}
At line:1 char:21

• git reset --soft @{1}

•                 ~
  

  Missing '=' operator after key in hash literal.
  At line:1 char:21
• git reset --soft @{1}

•                 ~
  

  The hash literal was incomplete.
  • CategoryInfo : ParserError: (:) [],
    ParentContainsErrorRecordEx
    ception
  • FullyQualifiedErrorId : MissingEqualsInHashLiteral

Huh?! Maybe my posh~git is different from yours...?

On Wed, Nov 16, 2016 at 2:06 PM, Dan Rumney [email protected]
wrote:

OK finally figured it out:

git reset --hard
git clean -df
git reset --soft @{1}
git commit -m "Rollback of breaking changes"
git push

I did this on my copy, ran npm install and npm test and everything
(except breader) seemed to work.

The breader issue is a line-endings issues due to OS differences.

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#78 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AATGJ4zO3efM4534-rzzio_IFH6vL8beks5q-1RMgaJpZM4KvycM
.

[dancrumb]

That's weird

If you're fine losing the README changes, this:

git reset --hard origin/master
git clean -df
git reset --hard <your commit of choice>
git reset --soft @{1}
git commit -m "Rollback of breaking changes"
git push

[jpaulm]

Took it! Phew! Thanks a million!

Onwards and upwards!

Paul

On Wed, Nov 16, 2016 at 4:52 PM, Dan Rumney [email protected]
wrote:

That's weird

If you're fine losing the README changes, this:

git reset --hard origin/master
git clean -df
git reset --hard
git reset --soft @{1}
git commit -m "Rollback of breaking changes"
git push

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#78 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AATGJ4xR9sUMjwBkaglYu4uWQjmzkvXBks5q-3sxgaJpZM4KvycM
.

[jpaulm]

Beautiful! Thanks! I guess these kinds of errors happen - I had a similar
problem with DrawFBP - although it was spotted faster!

Regards,

Paul

On Wed, Nov 16, 2016 at 1:05 PM, Dan Rumney [email protected]
wrote:

For skipping a test:

describe( /* everything */);

becomes

describe.skip( /* everything */);

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#78 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AATGJ31Ly6pdZQj2nAmp_xv8-76UpP-Eks5q-0XSgaJpZM4KvycM
.

[jpaulm]

Sorry, Dan, had to backup a bit further - to 99DA196 - before FIFO.js and
Connection.js were introduced. Still works, except that I have to skip
breader again - will do that tomorrow.

Readme should probably be checked.

Thanks for your help,

Paul

On Wed, Nov 16, 2016 at 4:52 PM, Dan Rumney [email protected]
wrote:

That's weird

If you're fine losing the README changes, this:

git reset --hard origin/master
git clean -df
git reset --hard
git reset --soft @{1}
git commit -m "Rollback of breaking changes"
git push

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#78 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AATGJ4xR9sUMjwBkaglYu4uWQjmzkvXBks5q-3sxgaJpZM4KvycM
.