Skip to content
This repository was archived by the owner on Oct 23, 2022. It is now read-only.

Fix subscription deadlock #130

Merged
merged 4 commits into from
Apr 1, 2020
Merged

Conversation

koivunej
Copy link
Collaborator

@koivunej koivunej commented Apr 1, 2020

This fixes the deadlock found when working on the WIP #126 code.

I saw you already refactored these subscription registries out and replaced them with oneshot channels. I think that is a good idea but didn't investigate it too deeply, as they'll handle dropping and cancelling issue better.

I'll follow up this with #126 new features on top of this.

Joonas Koivunen added 4 commits April 1, 2020 14:32
without this any future created by get_block will never complete, which
will hang http requests with the block api for example.
these help when logging, since we have limited number of subscription
types.
was thinking that leaving the waker there might need to be fixed but
apparently it causes no problems.
@koivunej
Copy link
Collaborator Author

koivunej commented Apr 1, 2020

Re-running workflow after the flaky pubsub test.. Strange that it never fails locally. I suspect it happens because there are >1 threads being used but not enough time for all of them.

@dvc94ch
Copy link
Contributor

dvc94ch commented Apr 1, 2020

LGTM, but shouldn't the exit_daemon be called after all futures are either complete or dropped?

@koivunej
Copy link
Collaborator Author

koivunej commented Apr 1, 2020

shouldn't the exit_daemon be called after all futures are either complete or dropped?

With the conformance tests for block/get called "should get a block added as CIDv0 with a CIDv1" it creates a block_get request which is we currently don't support finding, so it stays blocking forever, after the shutdown. The shutdown signal happens and the future resolves, but warp (or hyper) will wait for all spawned tasks to complete before exiting, which will never happen as the conformance test keeps the connection alive.

With the shutdown signal now forcing the subscription futures to complete the request will be dropped and warp will exit normally. It'll still take some time for the conformance tests to get an http client timeout (separate timeout from the 2s test timeout).

@koivunej koivunej mentioned this pull request Apr 1, 2020
@koivunej koivunej deleted the fix_subscription_deadlock branch September 24, 2020 12:56
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants