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

resource.getRequest().getCookies() doesn't work #485

Closed
PavelR577 opened this issue Jul 7, 2012 · 17 comments
Closed

resource.getRequest().getCookies() doesn't work #485

PavelR577 opened this issue Jul 7, 2012 · 17 comments
Labels

Comments

@PavelR577
Copy link

tested with version 1.0.0.beta2a (0.9.4 also) .

resource.getRequest().getCookies() - returns empty.
resource.getRequest().getHeader("cookie") - return the cookies (works)

@jfarcand
Copy link
Member

jfarcand commented Jul 7, 2012

It's websocket, right?

@PavelR577
Copy link
Author

yep - websocket.
tested on tomcat 7.0.28
windows xp/7 64bit

@kribblo
Copy link

kribblo commented Oct 15, 2012

I'm still seeing this on 1.0.1, Tomcat 7.0.30, Windows 7 64 bit. Using Websockets and looking for the cookies the first thing that happens in onRequest a class extending AbstractReflectorAtmosphereHandler. Using atmosphere-runtime 1.0.1 via maven.

getRequest().wrappedRequest().getCookies() is also empty, but getHeader("cookie") is properly populated.

@jfarcand
Copy link
Member

@kribblo Can you write a unit test? This test works so if you can find way to reproduce the issue I will take a look.

@kribblo
Copy link

kribblo commented Oct 15, 2012

I don't think there's a problem with that unit test, but it tests a manually populated cookie, not a list set by the server.

I think that the problem might be that Tomcat does not give the cookies as expected, so there is no cookies available from the start.

The request seems to be set as a HttpServletRequest, maybe it needs to get the WsHttpServletRequestWrapper - I don't know the inner workings and just use a bit of Google.

So I'm not sure I can make a unit test, sorry.


As a side note, wouldn't this code add the cookies again and again each time it's called (provided there were any cookies in the array :))

    @Override
    public Cookie[] getCookies() {
        Cookie[] c = b.request.getCookies();
        if (c != null && c.length > 0) {
            b.cookies.addAll(Arrays.asList(c));
        }
        return b.cookies.toArray(new Cookie[]{});
    }

@toddwest
Copy link
Contributor

I just wanted to note that I've been running into this exact same issue (even all the way up to version 1.0.2). My setup is pretty simple:

Chrome -> jquery.atmosphere.js -> Websocket -> Tomcat 7 (7.0.30)

One thing I was a little confused on is why in the cloneRequest method we are always calling this code:

b.request(r);

which effectively replaces the real request with a NoOpsRequest so the call to request.getCookies(); returns this:

@Override
public Cookie[] getCookies() {
    return new Cookie[0];
}

from the NoOpsRequest leaving us with what appears to be no cookies. Is there any way around this? I noticed that the JettyWebSocketUtil passes false (for loadInMemory) into the cloneRequest method so we'd never be replacing the request like this. Any tips you can provide for how to solve this would be great. Thanks!

@toddwest
Copy link
Contributor

Also, the unit test you linked above doesn't test the Tomcat7Websocket path and also doesn't include any of the Websocket headers when building up the request so it doesn't get to the problem code:

if (loadInMemory) {
    r = new NoOpsRequest();
    if (isWrapped) {
        load(b.request, b);
    } else {
        load(request, b);
    }
    b.request(r);
}

Hopefully I am just really misunderstanding/misconfiguring something here and this will be an easy fix.

@jfarcand jfarcand reopened this Oct 26, 2012
@jfarcand
Copy link
Member

@toddwest The cookie are loaded in memory by doing

        Cookie[] cs = request.getCookies();
        Set<Cookie> hs = new HashSet();
        for (Cookie c: cs) {
            hs.add(c);
        }
        b.cookies(hs);

In AtmosphereRequest. I've re-opened the issue to see what can be done.

@toddwest
Copy link
Contributor

Thanks for the reply! Yeah, I noticed that if I put a breakpoint right before this code:

b.request(r);

and make a call to request.getCookies(); it returns the full set of Cookies correctly. However if I make that call after that line of code it returns 0 cookies due to the NoOpsRequest replacing the real request. Look forward to seeing what you can find out! Thanks again!

jfarcand added a commit that referenced this issue Oct 26, 2012
@toddwest
Copy link
Contributor

Wow, thank you for the quick fix! I really appreciate it!

@kribblo
Copy link

kribblo commented Oct 27, 2012

Very nice! Will this get pushed to Maven anytime soon you think? (It's how we use the library).

@jfarcand
Copy link
Member

@kribblo It is as a SNAPSHOT and I will release 1.0.3 today to tomorrow. Thanks!

@kribblo
Copy link

kribblo commented Oct 29, 2012

Perfect! Thanks!

@jaltmanQ
Copy link

This seems to be happening again on Tomcat 8, as well as with Jetty 9.x on the latest version of atmosphere-runtime. From what I have seen I feel like anyone who is going through the JSR356AsyncSupport -> JSR356Endpoint is not getting cookies, but is getting a header with the cookies as a string.

@gvillegas
Copy link

It is happening again, I have been trying to also read request's attributes I set in a Filter before reaching a Managed Service, and none of them are found inside the resource's AtmosphereRequest. Could this issue be related, or is a normal behavior that Websocket transport drops these attributes before reaching the Managed Service?

BTW I found that someone already reported the cookies issue again: https://groups.google.com/forum/embed/?place=forum/atmosphere-framework&showsearch=true&showpopout=true&showtabs=false&parenturl=http%3A%2F%2Fasync-io.org%2Fdownload.html#!topic/atmosphere-framework/qbicJoaKmpU

Regards

@jaltmanQ
Copy link

jaltmanQ commented Apr 4, 2016

Should this be reopened or file a new issue?

@jfarcand
Copy link
Member

jfarcand commented Apr 4, 2016

@jaltman7 Reopen a new one ... with a fix :-)

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

No branches or pull requests

6 participants