Skip to content
This repository has been archived by the owner on Aug 23, 2020. It is now read-only.

Linked blocking queue #1833

Merged

Conversation

GalRogozinski
Copy link
Contributor

Description

Changes the queues that we use for networking to have unlimited capacity

Fixes #1742

Type of change

  • Bug fix (a non-breaking change which fixes an issue)

How Has This Been Tested?

IRI was synced without needing to restart and no error appeared
It should be noted that I did notice a slow down in syncing

I think we should investigate going back to array based queues later on to speed things up

Checklist:

  • My code follows the style guidelines for this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes

Copy link
Contributor

@DyrellC DyrellC left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think in the meantime until we know what process is thread locking the queue's on occasion, this is the best path forward to avoid the lockup that we saw earlier.

@kwek20
Copy link
Contributor

kwek20 commented Apr 9, 2020

Hmm yes as this will allocate memory for every add and remove, it is considerably heavier. I am not sure if its worth the drawbacks just to fix a minor shutdown error

@GalRogozinski
Copy link
Contributor Author

@kwek20
Should have made it clearer on the issue
It also causes syncing problems and clogs the node it caused devnet compass to stop and thus killed the network (see ops-general in our internal slack), which is quite worrisome

Having said this, I agree with you...
It is also why I was reluctant to merge my fix, that was sitting on my branch for quite a while (you can see a reference to it in the issue).

My plan was to open another issue about changing it back to array based queue but to solve the deadlock. If you think you can have a look in the meanwhile and create the real solution it will be great. I have no idea how easy/hard it will be...

@kwek20
Copy link
Contributor

kwek20 commented Apr 9, 2020

I have asked @luca-moser for feedback on the original decision. Might give us some insights.

I will open the issue to solve this properly, and then approve

@GalRogozinski GalRogozinski merged commit 7d97993 into iotaledger:release-v1.9.0 Apr 9, 2020
@luca-moser
Copy link
Member

As per Brord's request: the sole reason I made them ArrayBlockingQueues was to have them bounded. I believe the source problem is the incorrect handling of shutdown events in conjunction in which order the different stages are interrupted. The first crash log from the linked issue suggests that there's no real interrupt handler in the BCTCurl (didn't have a closer look at the code though).

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants