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.

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

handle redirect with processors #593

Merged
merged 2 commits into from Apr 25, 2021

Conversation

philippe44
Copy link
Contributor

@philippe44 philippe44 commented Apr 21, 2021

There is an issue when using "processors" (e.g. mp4 => aac) and the HTTP requests redirect. The on-the-fly created header (initialAudioBlock) is overwritten by the recursive calls in S::P::P::HTTP.pm::request.

This is a 8.1 patch that does not need to be ported to 8.2 as this PR includes it as well

@philippe44 philippe44 changed the title handle redirect in processors handle redirect with processors Apr 21, 2021
Copy link
Member

@michaelherger michaelherger left a comment

Choose a reason for hiding this comment

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

My Perl isn't good enough. Please help me!

Comment on lines +81 to +82

$song->initialAudioBlock('') unless defined $song->initialAudioBlock;
Copy link
Member

Choose a reason for hiding this comment

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

Wouldn't this only be executed if the above condition was true? Therefore could be moved up there?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure. There could be many reasons why above he initialAudioBlock could still be uninitialized and I want to make sure it does not remains undef. What am I missing?

$self = $self->SUPER::request($args);
return unless $self;
return $self if !$self || exists ${*$self}{'initialAudioBlockRef'};
Copy link
Member

Choose a reason for hiding this comment

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

I need some help here: what exactly does the second condition do?...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It checks that is there in the hash. It is the way to know if we are the first call in a chain of redirection or not. Because we must only execute the code underneath for the actual used request, not for the redirect one. We could have 5 redirections and so that code will be called 6 times, recursively. When unfolding, the $self will always be the same but only the very inner call will be useful, and this is the only one with non-existing 'initialAudioBlockRef".

See S::F::RemoteStream::request

if (my $redir = $self->redirect) {
		# Redirect -- maybe recursively?

		# Close the existing handle and refcnt-- to avoid keeping the
		# socket in a CLOSE_WAIT state and leaking.
		$self->close();

		# some 302 location omit the protocol, take it from original url
		$redir = Slim::Utils::Misc::cloneProtocol($redir, $url);
		main::INFOLOG && $log->info("Redirect to: $redir");

		return $class->new({
			'url'     => $redir,
			'song'    => $args->{'song'},
			'infoUrl' => $self->infoUrl,
			'post'    => $post,
			'create'  => $args->{'create'},
			'client'  => $args->{'client'},
		});
	}

@mherger mherger merged commit 29d3f9d into LMS-Community:public/8.1 Apr 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants