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

Send static file action is invoked before applying the relevant headers [SPR-17348] #21882

Closed
spring-issuemaster opened this issue Oct 5, 2018 · 5 comments
Assignees

Comments

@spring-issuemaster
Copy link
Collaborator

@spring-issuemaster spring-issuemaster commented Oct 5, 2018

Violeta Georgieva opened SPR-17348 and commented

Hi,

There is the following code for sending static file in

org.springframework.http.server.reactive.ReactorServerHttpResponse
public Mono<Void> writeWith(File file, longposition, longcount) {
    return doCommit(() -> this.response.sendFile(file.toPath(), position, count).then());
}

protected Mono<Void> doCommit(@Nullable Supplier<? extends Mono<Void>> writeAction) {
    if (!this.state.compareAndSet(State.NEW, State.COMMITTING)) {
        if (logger.isDebugEnabled()) {
            logger.debug("Skipping doCommit (response already committed).");
        }
        return Mono.empty();
    }

    this.commitActions.add(() ->
        Mono.fromRunnable(() -> {
            applyStatusCode();
            applyHeaders();
            applyCookies();
            this.state.set(State.COMMITTED);
    }));

    if (writeAction != null) {
        this.commitActions.add(writeAction);
    }

    List<? extends Mono<Void>> actions = this.commitActions.stream()
.map(Supplier::get).collect(Collectors.toList());

    return Flux.concat(actions).then();
}

As a result of

.map(Supplier::get)

,

this.response.sendFile(file.toPath(), position, count)

will be executed before applying the relevant headers.
In case there is a compression predicate that uses the information from the headers in order to instruct Reactor Netty whether to compress or not then the predicate may return wrong instructions because of the missing headers.

Original issue is reported here reactor/reactor-netty#430
 
Can you please check this?

Thanks in a advance,
Violeta


Affects: 5.0.9

Issue Links:

  • #21138 WebFlux commitActions are called after status, headers and cookies are set
@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

@spring-issuemaster spring-issuemaster commented Oct 5, 2018

Rossen Stoyanchev commented

Upon taking a closer look ... I do not see where in the code Reactor Netty checks for the values of headers, but shouldn't that check be deferred? Given that sendFile is a declaration for the sending, and it returns a Mono<Void>, I would have thought it doesn't do much until there is a subscriber, or does it actually need to check some headers at declaration time?

The reason I ask is because we have similar calls for send and sendGroups.] I'm wondering if those would need to be deferred also?

 

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

@spring-issuemaster spring-issuemaster commented Oct 5, 2018

Rob Maskell commented

Maybe some more info from Andy Wilkinson on this ticket in case you didn't see it spring-projects/spring-boot#12892

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

@spring-issuemaster spring-issuemaster commented Oct 5, 2018

Violeta Georgieva commented

hm good point let me check

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

@spring-issuemaster spring-issuemaster commented Oct 5, 2018

Violeta Georgieva commented

You are right. I reopened the issue and also created a PR for it. reactor/reactor-netty#451

Fill free to close this issue.

Thanks for the input,

Violeta

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

@spring-issuemaster spring-issuemaster commented Oct 5, 2018

Rossen Stoyanchev commented

Okay, thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
2 participants
You can’t perform that action at this time.