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

Use the Request context instead of a blank one #309

Closed
wants to merge 1 commit into from

Conversation

butonic
Copy link

@butonic butonic commented Oct 4, 2019

When trying to store eg the authenticated user in the context the Unrouted handler currently drops the context when handling tus requests, this prevents implementing a storage that needs to access that user.

I'm not sure about the writeChunk() but these should use the context from the request (also to pass on tracing spans).

From the request docs:

// Context returns the request's context. To change the context, use
// WithContext.
//
// The returned context is always non-nil; it defaults to the
// background context.
//
// For outgoing client requests, the context controls cancelation.
//
// For incoming server requests, the context is canceled when the
// client's connection closes, the request is canceled (with HTTP/2),
// or when the ServeHTTP method returns.
func (r *Request) Context() context.Context {
	if r.ctx != nil {
		return r.ctx
	}
	return context.Background()
}

When trying to store eg the authenticated user in the context the Unrouted handler currently drops the context when handling tus requests, this prevents implementing a storage that needs to access that user.
@Acconut
Copy link
Member

Acconut commented Oct 5, 2019

We originally passed the request's context to the data store but reverted it in 65d2f1c and 57be489 for two reasons:

  • As you also assumed, the request context must not be passed to WriteChunk or its impossible to save the data transferred in an aborted PATCH request, which defeats the purpose of resumability.
  • We also received a number of "request context cancelled" errors for POST requests when we deployed tusd to master.tus.io. Even after investigating it for longer, I wasn't able to find the root cause (it might be caused by the nginx configuration but that also looked fine). Therefore, we deemed it too dangerous for production usage and changed the contexts to context.Background().

That being said, I would still love to use the request context but I wasn't able to get it working properly.

@butonic
Copy link
Author

butonic commented Oct 6, 2019

AFAIU it should be possible to use a sub context for the stopUpload part... which is called when the server decides to terminate the upload due to a timeout (or probably other reasons)? That should allow writing the receved data ... Let me test that this week in our scenario.

Hm ... "request context cancelled" came up in which service log? If an nginx decided to kill a request you would still want to store the bytes ... if it was a POST with body ... which would be the same problem as the previous one.

I'm not sure we will be using the create extension ... so we might not run into you POST issue ... still ... let me dig into this. Will report back.

@Acconut
Copy link
Member

Acconut commented Oct 7, 2019

AFAIU it should be possible to use a sub context for the stopUpload part... which is called when the server decides to terminate the upload due to a timeout (or probably other reasons)? That should allow writing the receved data ... Let me test that this week in our scenario.

It's not primarily about stopping an upload but we had problems when the user wants to pause an upload. In these situations, the client will usually just abort the PATCH request. This causes the Go's HTTP server to cancel the request context. The data stores should still store the upload in this situation but cannot use the request context because it is already cancelled.

Hm ... "request context cancelled" came up in which service log? If an nginx decided to kill a request you would still want to store the bytes ... if it was a POST with body ... which would be the same problem as the previous one.

It was a very weird error, I can explain more if you want. What's unexplainable to me is that the tus clients got a error response saying "request context cancelled". This means that the Go decided to end the request context but nginx still had a valid connection to tusd or otherwise the tus client would not have received the response. The POST requests didn't contain a body and it was occurring randomly, most POST requests completed fine.

I'm not sure we will be using the create extension ... so we might not run into you POST issue

I am afraid that this is not only limited to POST requests but should also happen to HEAD requests, for example.

@butonic
Copy link
Author

butonic commented Oct 10, 2019

closing in favor of #315

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.

2 participants