Skip to content
This repository has been archived by the owner on Nov 20, 2018. It is now read-only.

Add response trailer feature #1000

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
// Copyright (c) .NET Foundation. All rights reserved.
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.

using System;
using System.Threading.Tasks;

namespace Microsoft.AspNetCore.Http.Features
{
public interface IHttpResponseTrailersFeature
{
/// <summary>
/// The response trailers to send.
/// </summary>
IHeaderDictionary Trailers { get; set; }

/// <summary>
/// Indicates if the server has started writing the response trailers.
/// If true, the <see cref="Trailers"/> are now immutable, and <see cref="OnStarting"/> should no longer be called.
/// </summary>
bool HasStarted { get; }
Copy link
Member

Choose a reason for hiding this comment

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

WasWritten?

Copy link
Contributor Author

@khellang khellang Feb 23, 2018

Choose a reason for hiding this comment

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

That would indicate that it's set after the trailers has been written? This would be set just before the server starts writing.

Copy link
Member

Choose a reason for hiding this comment

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

I'm not too convinced WasWritten is a better name either.

I proposed WasWritten since the Trailers dictionary is closed for modification and the server has committed to writing what's in the dictionary as trailers at the point this property is set. From the perspective of the app developer, I don't see how committing to writing the trailers is observably different from actually writing the trailers especially given that there's write-behind response buffer.

HasStarted isn't bad though. I was thinking HasStartedWriting to make it less ambiguous, but I suggested WasWritten since that's shorter.

If we decided to change OnStarting to OnStartingTrailers or something similar, we might want to change this to HasStartedTrailers instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agan, I chose HasStarted because it's consistent with IHttpResponseFeature.HasStarted. If we can find another name, or get rid of the property entirely, that's cool with me.

Copy link
Member

Choose a reason for hiding this comment

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

IsReadOnly? IHeaderDictionary already has that property, we can probably remove this one.

Copy link

@serialseb serialseb Mar 1, 2018

Choose a reason for hiding this comment

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

Why would the trailer dictionary not be mutable? I think I'm missing something in the IsReadOnly comment

Ok, got it. On the trailer header dictionary. Makes sense.


/// <summary>
/// Registers a callback to be invoked just before writing the response trailers.
/// This is the last chance to modify the <see cref="Trailers"/>.
/// </summary>
/// <param name="callback">The callback to invoke before writing the trailers.</param>
/// <param name="state">The state to pass into the callback.</param>
void OnStarting(Func<object, Task> callback, object state);
Copy link
Member

Choose a reason for hiding this comment

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

Nit: Maybe call this OnWritingResponseTrailers.

I get that my suggested name is a little redundant with the name of the interface, but I think the current naming is too similar to IHttpResponseFeature.OnStarting and could cause confusion.

Choose a reason for hiding this comment

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

There's already an .OnSendingHeaders, maybe an .OnSendingTrailerHeaders would work?

Copy link
Contributor Author

@khellang khellang Feb 23, 2018

Choose a reason for hiding this comment

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

There's an .OnSendingHeaders? I think that's the OWIN key. It's called OnStarting in the ASP.NET Core abstractions. That's why I chose this name, for consistency. I'm fine with whatever.

/// <summary>
/// Registers a callback to be invoked just before the response starts. This is the
/// last chance to modify the <see cref="Headers"/>, <see cref="StatusCode"/>, or
/// <see cref="ReasonPhrase"/>.
/// </summary>
/// <param name="callback">The callback to invoke when starting the response.</param>
/// <param name="state">The state to pass into the callback.</param>
void OnStarting(Func<object, Task> callback, object state);

Choose a reason for hiding this comment

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

Ah. Yes I see what you mean. Shame it wasn't called something more like the owin feature then. Consistency would probably win here.

}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
// Copyright (c) .NET Foundation. All rights reserved.
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.

using System;
using System.Threading.Tasks;

namespace Microsoft.AspNetCore.Http.Features
{
public class HttpResponseTrailersFeature : IHttpResponseTrailersFeature
Copy link
Member

Choose a reason for hiding this comment

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

Where do you expect to need this implementation? I expect the servers to provide their own.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Where do you expect to need this implementation?

I don't. I just saw the other features had "dummy" implementations, so just followed along.

{
public HttpResponseTrailersFeature()
{
Trailers = new HeaderDictionary();
}

public IHeaderDictionary Trailers { get; set; }

public virtual bool HasStarted => false;

public virtual void OnStarting(Func<object, Task> callback, object state)
{
}
}
}