Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Update Membrane.Pipeline.terminate/2 behavior #519

Merged
merged 6 commits into from
Feb 14, 2023
Merged

Update Membrane.Pipeline.terminate/2 behavior #519

merged 6 commits into from
Feb 14, 2023

Conversation

FelonEkonom
Copy link
Member

@FelonEkonom FelonEkonom force-pushed the update-pipeline-terminate branch 2 times, most recently from 8620fb7 to a94458d Compare January 30, 2023 16:20
@FelonEkonom FelonEkonom added the no-changelog This label has to be added if changes from the PR are not meant to be placed in the CHANGELOG.md label Jan 30, 2023
@FelonEkonom FelonEkonom force-pushed the update-pipeline-terminate branch from a94458d to ad225d4 Compare January 30, 2023 16:21
@FelonEkonom FelonEkonom marked this pull request as ready for review January 30, 2023 16:22
@FelonEkonom FelonEkonom requested a review from mat-hek as a code owner January 30, 2023 16:22
@FelonEkonom FelonEkonom requested a review from varsill February 1, 2023 12:39
@FelonEkonom FelonEkonom force-pushed the update-pipeline-terminate branch 2 times, most recently from e85ceb6 to fa8d515 Compare February 2, 2023 16:31
@FelonEkonom FelonEkonom force-pushed the update-pipeline-terminate branch from fa8d515 to 652d066 Compare February 2, 2023 16:35
mat-hek
mat-hek requested changes Feb 7, 2023
View reviewed changes
lib/membrane/pipeline.ex Outdated
* `timeout` - tells how much time (ms) to wait for pipeline to get gracefully
terminated. Defaults to 5000.
* `force?` - if set to `true` and pipeline is still alive after `timeout`,
pipeline will be killed using `Process.exit/2` with second argument set to
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
pipeline will be killed using `Process.exit/2` with second argument set to
pipeline will be killed using `Process.exit/2` with reason

lib/membrane/pipeline.ex Outdated
* `asynchronous?` - if set to `true`, pipline termination won't be blocking and
will be executed in the process, which pid is returned as function result. If
set to `false`, pipeline termination will be blocking and will be executed in
the process, that called this function.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
the process, that called this function.
the process that called this function.

lib/membrane/pipeline.ex Outdated

ref = if blocking?, do: Process.monitor(pipeline)
Message.send(pipeline, :terminate)
{asynchronous?, opts} = Keyword.pop(opts, :asynchronous?, false)
Copy link
Member

Choose a reason for hiding this comment

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

let's use Keyword.validate! to get all the options

lib/membrane/pipeline.ex Outdated
Process.exit(pipeline, :kill)
{:error, :timeout}
else
raise PipelineTerminationError, """
Copy link
Member

Choose a reason for hiding this comment

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

Having a separate error for that is an overkill, let's raise PipelineError

lib/membrane/pipeline.ex Outdated
end
@spec run_terminate(pipeline :: pid, timeout: timeout(), force?: boolean()) ::
:ok | {:error, :timeout}
def run_terminate(pipeline, opts) do
Copy link
Member

Choose a reason for hiding this comment

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

This should have @doc false. do_terminate would be a better name too.

varsill
varsill requested changes Feb 7, 2023
View reviewed changes
Copy link
Contributor

@varsill varsill left a comment

Choose a reason for hiding this comment

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

One more thing - I haven't seen the usage of the force?: true option anywhere in the tests. Perhaps we should check if it's working with that option properly

lib/membrane/pipeline.ex Outdated Show resolved Hide resolved
lib/membrane/pipeline.ex Show resolved Hide resolved
lib/membrane/pipeline.ex Outdated
"""
@spec terminate(pipeline :: pid, Keyword.t()) ::
:ok | {:error, :timeout}
@spec terminate(pipeline :: pid, timeout: timeout(), force?: boolean()) ::
Copy link
Contributor

Choose a reason for hiding this comment

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

You have missed the asynchronous? option in the spec

lib/membrane/pipeline.ex Outdated
def call(pipeline, message, timeout \\ 5000) do
GenServer.call(pipeline, message, timeout)
end
@spec run_terminate(pipeline :: pid, timeout: timeout(), force?: boolean()) ::
Copy link
Contributor

Choose a reason for hiding this comment

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

That one should be somehow made "private". I known you have made it public since you can use it both directly and with the asynchronous Task, but now we will have Membrane.Pipeline.run_terminate/2 function in the API, with a defective options list ;D

lib/membrane/rc_pipeline.ex
@@ -392,7 +392,7 @@ defmodule Membrane.RCPipeline do
def handle_terminate_request(_ctx, state) do
pipeline_event = %Terminated{from: self()}
send_event_to_controller_if_subscribed(pipeline_event, state)
{[], state}
{[terminate: :normal], state}
Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, that's odd that that one wasn't terminating :O

@FelonEkonom FelonEkonom force-pushed the update-pipeline-terminate branch from 6542813 to b0055fc Compare February 9, 2023 15:04
@FelonEkonom FelonEkonom force-pushed the update-pipeline-terminate branch from 8ecb4ae to c44f4b9 Compare February 14, 2023 12:12
@FelonEkonom FelonEkonom merged commit e61fd02 into master Feb 14, 2023
@FelonEkonom FelonEkonom deleted the update-pipeline-terminate branch February 14, 2023 12:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mat-hek mat-hek mat-hek approved these changes

@varsill varsill varsill approved these changes

Assignees
No one assigned
Labels
no-changelog This label has to be added if changes from the PR are not meant to be placed in the CHANGELOG.md
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@FelonEkonom @varsill @mat-hek