When you delete the reviewer, the UI will be refreshed via websocket. Seems to me that websocket has some issues posting changes to your browser. Is OneDev running behind a reverse proxy? If so, make sure to configure the proxy to forward websocket connections:

https://docs.onedev.io/administration-guide/reverse-proxy-setup

Also when you added someone as reviewer, merge button will only appear after the user approves the pull request.

The assignee is the one designated to merge the pull request, same as GitHub concept.

Thank you very much @robin for the explanations.

If so, make sure to configure the proxy to forward websocket connections:

Yes it is running behind an Apache proxy since native HTTPS support is no longer available. I was missing loading the proxy_wstunnel_module and the two ProxyPass directives to ws://. However, even after adding these and restarting Apache, I am struggling to get this to work. I still get 404 errors in my access log:

"GET /wicket/websocket?pageId=163&wicket-ajax-baseurl=project%2F~builds%3Fquery%3D%2522Status%2522%2Bis%2B%2522Failed%2522&wicket-app-name=DefaultWicketServlet%24%24EnhancerByGuice%24%24160417877 HTTP/1.0" 404 3508

I spent a lot of time to try to find out why it's not working but to no avail.

Also when you added someone as reviewer, merge button will only appear after the user approves the pull request.

In this case, the Pull Request was in "Request for Changes" mode after being in Review mode. After clicking that button, the Approve button disappears, and the only way to move it away from that "Request for Changes" mode is to remove the reviewer, but this not straightforward at all. This is a confusing bug / severe limitation in my opinion. The "approve" button should probably remain, or a "cancel request for changes" button could appear.

The assignee is the one designated to merge the pull request, same as GitHub concept.

On GitHub, anyone with write access to the repo can merge the pull request, regardless of who it is assigned to (I just verified that this is the case). It is confusing because we may want to use the assignees as the person in charge of moving this PR forward i.e., to make the requested changes. I assume it is intended to be the person who originally submitted the pull request, but sometimes we may want to re-assign the PR to someone else to make those requested changes.

NOTE: I still think those 500 server errors should not occur for clients with broken WebSockets using an outdated status if the lack of update is the source of that problem. They should be caught as a 4xx client error if the client is submitting an invalid request. Technically, I guess it could still happen with working websockets if the user clicks again on a button before the page is updated and it disappears.

Let's get the websocket working before talking about other topics. The 404 error should not occur. Seems that there is something wrong with your front end proxy. Or to narrow down the issue, you may try with other options such as Nginx or Caddy server.

@robin I tried to investigate this more. Even with a trace2 LogLevel, everything in the onedev-error_log seems to indicate that the WebSocket proxying is working properly, and that it is really OneDev returning a 404.

Also it was pointed out to me that the access control directives from the Apache proxying guide are outdated 2.2 directives. I have the same result with or without them though.

[Mon Mar 27 23:51:53.561757 2023] [proxy:trace2] [pid 24148:tid 140277465384640] mod_proxy.c(884): [client <ip address>:55830] AH03461: attempting to match URI path '/wicket/websocket' against prefix '/wicket/websocket' for proxying
[Mon Mar 27 23:51:53.561767 2023] [proxy:trace1] [pid 24148:tid 140277465384640] mod_proxy.c(1001): [client <ip address>:55830] AH03464: URI path '/wicket/websocket' matches proxy handler 'proxy:ws://localhost:6610/wicket/websocket'
[Mon Mar 27 23:51:53.561778 2023] [authz_core:debug] [pid 24148:tid 140277465384640] mod_authz_core.c(843): [client <ip address>:55830] AH01628: authorization result: granted (no directives)
[Mon Mar 27 23:51:53.561789 2023] [proxy_http:trace1] [pid 24148:tid 140277465384640] mod_proxy_http.c(97): [client <ip address>:55830] HTTP: canonicalising URL ws://localhost:6610/wicket/websocket
[Mon Mar 27 23:51:53.561812 2023] [proxy:trace2] [pid 24148:tid 140277465384640] proxy_util.c(2340): [client <ip address>:55830] ws: found worker ws://localhost:6610/wicket/websocket for ws://localhost:6610/wicket/websocket?pageId=13&wicket-ajax-baseurl=&wicket-app-name=DefaultWicketServlet%24%24EnhancerByGuice%24%24160417877
[Mon Mar 27 23:51:53.561820 2023] [proxy:debug] [pid 24148:tid 140277465384640] mod_proxy.c(1506): [client <ip address>:55830] AH01143: Running scheme ws handler (attempt 0)
[Mon Mar 27 23:51:53.561826 2023] [proxy_http:trace1] [pid 24148:tid 140277465384640] mod_proxy_http.c(1887): [client <ip address>:55830] HTTP: serving URL ws://localhost:6610/wicket/websocket?pageId=13&wicket-ajax-baseurl=&wicket-app-name=DefaultWicketServlet%24%24EnhancerByGuice%24%24160417877
[Mon Mar 27 23:51:53.561834 2023] [proxy:debug] [pid 24148:tid 140277465384640] proxy_util.c(2538): AH00942: ws: has acquired connection for (localhost:6610)
[Mon Mar 27 23:51:53.561862 2023] [proxy:debug] [pid 24148:tid 140277465384640] proxy_util.c(2596): [client <ip address>:55830] AH00944: connecting ws://localhost:6610/wicket/websocket?pageId=13&wicket-ajax-baseurl=&wicket-app-name=DefaultWicketServlet%24%24EnhancerByGuice%24%24160417877 to localhost:6610
[Mon Mar 27 23:51:53.561872 2023] [proxy:debug] [pid 24148:tid 140277465384640] proxy_util.c(2819): [client <ip address>:55830] AH00947: connected /wicket/websocket?pageId=13&wicket-ajax-baseurl=&wicket-app-name=DefaultWicketServlet%24%24EnhancerByGuice%24%24160417877 to localhost:6610
[Mon Mar 27 23:51:53.561981 2023] [proxy:debug] [pid 24148:tid 140277465384640] proxy_util.c(3120): AH00951: ws: backend socket is disconnected.
[Mon Mar 27 23:51:53.562009 2023] [proxy:trace2] [pid 24148:tid 140277465384640] proxy_util.c(3257): ws: fam 10 socket created to connect to localhost:6610
[Mon Mar 27 23:51:53.562102 2023] [proxy:debug] [pid 24148:tid 140277465384640] proxy_util.c(3291): AH02824: ws: connection established with [::1]:6610 (localhost:6610)
[Mon Mar 27 23:51:53.562136 2023] [proxy:debug] [pid 24148:tid 140277465384640] proxy_util.c(3480): AH00962: ws: connection complete to [::1]:6610 (localhost)
[Mon Mar 27 23:51:53.577273 2023] [headers:trace2] [pid 24148:tid 140277465384640] mod_headers.c(865): AH01502: headers: ap_headers_output_filter()
[Mon Mar 27 23:51:53.577329 2023] [proxy:debug] [pid 24148:tid 140277465384640] proxy_util.c(2554): AH00943: ws: has released connection for (localhost:6610)
[Mon Mar 27 23:51:53.577385 2023] [proxy_http:trace2] [pid 24148:tid 140277465384640] mod_proxy_http.c(1786): [client <ip address>:55830] end body send
[Mon Mar 27 23:51:53.577481 2023] [ssl:debug] [pid 24148:tid 140277465384640] ssl_engine_io.c(1147): [client <ip address>:55830] AH02001: Connection closed to child 152 with standard shutdown (server site.example.com:443)
[Mon Mar 27 23:51:53.730315 2023] [ssl:debug] [pid 24148:tid 140277524133568] ssl_engine_io.c(1147): [client <ip address>:55820] AH02001: Connection closed to child 145 with standard shutdown (server site.example.com:443)

Yet the onedev-access_log will get a:

<ip address> - - [27/Mar/2023:23:51:53 -0400] "GET /wicket/websocket?pageId=13&wicket-ajax-baseurl=&wicket-app-name=DefaultWicketServlet%24%24EnhancerByGuice%24%24160417877 HTTP/1.0" 404 3505

@robin I managed to set up nginx with the same config but enabling SSL on a different port, and I have the exact same 404 WebSocket problem.

My config says:

   server {
     listen 44444 ssl;
     listen [::]:44444 ssl;
     ssl_certificate /etc/letsencrypt/live/site.example.com/fullchain.pem;
     ssl_certificate_key /etc/letsencrypt/live/site.example.com/privkey.pem;
     server_name site.example.com;
     client_max_body_size 100M; 
     location /wicket/websocket {
             proxy_pass http://localhost:6610/wicket/websocket;
             proxy_http_version 1.1;
             proxy_set_header Upgrade $http_upgrade;
             proxy_set_header Connection "upgrade";                
     }
     location /~server {
             proxy_pass http://localhost:6610/~server;
             proxy_http_version 1.1;
             proxy_set_header Upgrade $http_upgrade;
             proxy_set_header Connection "upgrade";
     }
     location / {
             proxy_pass http://localhost:6610/;
     }
   }

With both Apache and nginx, the browser will report:

Error during WebSocket handshake: Unexpected response code: 404

and the access log will report:

<ip address> - - [28/Mar/2023:00:33:52 -0400] "GET /wicket/websocket?pageId=31&wicket-ajax-baseurl=&wicket-app-name=DefaultWicketServlet%24%24EnhancerByGuice%24%24160417877 HTTP/1.0" 404 3505 "-" "Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/110.0.0.0 Safari/537.36"

Is there anyway to increase the verbosity of these OneDev logs to debug this better?

Note that I am proxying through https (which is the whole point of the proxy).

Seems that these proxy entries are not taking effect for some reason.

I just set up a EC2 instance to run Apache httpd with OneDev, and websocket works without any issues. To narrow down the issue, can you please turn off SSL for the host to see if it works?

The WebSocket seems to be working correctly if interacting directly with the host at port 6610, and also if doing the proxy without SSL. But that defeats the purpose of the proxy ;) Not sure what is going on with that SSL...

Possibly related:

https://stackoverflow.com/questions/51622101/apache-config-websockets-proxy-wss-request-to-ws-backend

https://stackoverflow.com/questions/31211828/apache-proxy-websocket-wss-to-ws

At my side, no need to worry anything from wss:// to ws://, both for nginx and apache. The proxy handled it automatically. Maybe you are on an old apache version?

@robin Apache/2.4.55 which is just one version behind and nginx/1.22.1 . The normal HTTP stuff is poxying just fine, only having issues with this 404 on the wss:// requests. But as I mentioned, from the trace2 logs it "looks" like it is being proxied properly. I will try even higher log levels, since they seem to report more SSL information.

I made another test on a new RHEL instance with Apache 2.4.53, enabled SSL and also websocket works fine. You may test the setup on a new EC2 instance, and compare with your SSL setting to see if there is any obivous difference.

@robin So I investigated this a lot more, and it really seems like it does get to OneDev, but for some reason OneDev perhaps is confused and does not treat it like a WebSocket, or doesn't like something about the request, when it originated from a proxied wss:// request?

OneDev returns a panda "I didn't eat it. I swear!" message and a

<div class="websocket-error">Page is in error, reload to recover</div>.

This is with both nginx and Apache2.

I am testing with curl:

curl -v 'https://example.com/wicket/websocket?pageId=32&wicket-ajax-baseurl=project%2F~pulls%2F2&wicket-app-name=DefaultWicketServlet%24%24EnhancerByGuice%24%24160417877' -H 'Origin: https://example.com' -H 'Sec-WebSocket-Key: o7E+ka4Q1mh7Cm5Q1dCntg=='  -H 'Upgrade: websocket' -H 'Connection: Upgrade' -H 'Sec-WebSocket-Version: 13'

Update: digging into this deeper, for the Apache proxy, I realized that for HTTPS (which does not work) I have the following header:

 Connection: Keep-Alive

whereas for HTTP I have:

Connection: Upgrade
Upgrade: websocket

I know what's going on. It's my first level proxy that already lost those headers.

I think I finally got this working with Apache...

On top of having to fix both proxies to proxy WebSockets properly, I had to set ProxyWebsocketFallbackToProxyHttp Off on both of them. From https://httpd.apache.org/docs/2.4/mod/mod_proxy_wstunnel.html:

Since httpd 2.4.47, mod_proxy_http can handle WebSocket upgrading and tunneling in accordance to RFC 7230, this directive controls whether mod_proxy_wstunnel should hand over to mod_proxy_http to this, which is the case by default. Setting to Off lets mod_proxy_wstunnel handle WebSocket requests as in httpd 2.4.46 and earlier.

It seems that this HTTP fallback does not work with SSL.

Essentially, in Apache 2.4.47 and later, the ws tunnel module is disabled without this option set. Looks like there may be a newer way to do this without the rules rewriting:

ProxyPass "/some/ws/capable/path/" "http://example.com/some/ws/capable/path/" upgrade=websocket

So here are two configurations that do work with Apache 2.4.47+:

  ProxyWebsocketFallbackToProxyHttp Off
  ProxyPass /wicket/websocket ws://localhost:6610/wicket/websocket
  ProxyPass /~server ws://localhost:6610/~server
  ProxyPass / http://localhost:6610/

or

  ProxyWebsocketFallbackToProxyHttp Off
  ProxyPass / http://localhost:6610/
  RewriteEngine On
  RewriteCond %{HTTP:Upgrade} websocket [NC]
  RewriteCond %{HTTP:Connection} Upgrade [NC]
  RewriteRule ^/?(.*) "ws://localhost:6610/$1" [P,L]

I thought the following could work instead using the new way, but i get Error during WebSocket handshake: Invalid status line errors with this:

  ProxyPass /wicket/websocket http://localhost:6610/wicket/websocket upgrade=websocket
  ProxyPass /~server http://localhost:6610/~server upgrade=websocket
  ProxyPass / http://localhost:6610/

It would be good to update the reverse proxy guide with this missing information. I wonder why it worked for you, possibly ProxyWebsocketFallbackToProxyHttp is automatically disabled on those enterprise Linux environments since it's such a breaking change.

EDIT: I just found another reason why things may not have been working. In my first proxy I had:

SetEnv force-proxy-request-1.0 1

and as I learned from https://bz.apache.org/bugzilla/show_bug.cgi?id=65294 WebSocket upgrades should be ignored with HTTP 1.0.

Glad to see it is working for you now. The default setting works on EC2 Ubuntu, RHEL and My Mac as well. I will experiment with options you suggested and improve the manual as necessary.

Thanks again for the detailed feedback.

Thank you for improving the manual and your help investigating this.

Now let's talk about the pull request workflow confusing you. I guess you are stumbled upon below screen:

You were welcomed with this screen because one of the reviewer (robin in this screenshot) reviewed the pull request and feels that something needs to be improved. At this point you can only add more commits to the pull request for the reviewer to review, or contact the reviewer to change their mind and approve the pull request. Only when all reviewers approve the pull request, you can then merge it. If some reviewer is not possible to approve the pull request for some reason, you may simply remove that reviewer.

@robin Yes except I was the reviewer (robin) who asked for changes.

What I would like to see is a way to either "Cancel request for changes", or keep seeing the "Approve" button even though I asked for changes. I am not sure what is the view of the person who opened the pull request, but it seems off that the only way to be able to Approve it is to remove myself as the reviewer and add myself again to approve it. So much so, that I was wondering whether I should try clicking "Discard" to unstuck this PR from that request for changes.

And I was not the assignee, but as admin of the repo I would like to always see the "Merge" button, regardless of who is assigned.

You may click the refresh icon to take back your decision (request for changes):

And you can then approve the pull request to make it mergeable. However you still need to be pull request assignee to merge the pull request. It is designed to be so as at each phase, it is clear who is responsible for the pull request, for instance there is no excuse for assignee to not merge pull request as no other person can merge it once assigned.

If you as repository owner want to merge the pull request, just add yourselve as one of the assignee and proceed.

Also the discard button is the same as close in GitHub. Since merged and discarded are all closed pull requests in OneDev, using close is a little confusing to my understanding.

@robin OK. That refresh button is what I was essentially looking for, but in my opinion, it is extremely confusing because:

• It is a small button unlike the bigger Approve / Request for Changes buttons that controls the main flow of the PR,
• It is on the side, not at the same place as the bigger Approve / Request for Changes buttons that controls the main flow of the PR,
• The tooltip says "Request Review" (Why would I click this? It's already under review...)
• The Refresh Icon is not used in any way that I could understand that this cancels my request for change.

I mostly understood Discard as what it is, except for the fact that it is under that Orange notification that I want to "Discard" and that I could not find any other way to cancel the request for changes.

Request review means to request particular user to re-review the pull request. It is necessary but not very frequently used in normal workflow. I admit that for current user, it is a bit confusing. Will think more about this...

Thanks @robin .

So you mean the refresh button is also used to send a notification to that particular reviewer? The "refresh" icon makes more sense in that way. But that seems a different functionality than this "cancel request for change".

So perhaps a separate "Cancel request for change" button could be added to the current reviewer where the Approve / Request changes buttons are located.

The main reason I filed this issue though was because the server returned an "Oops!" error (I believe it was a 500 error), which really should never happen except for uncaught bugs (an IndexOutOfBoundsException error in this case). 5xx HTTP code means "I (the server) messed up", and it's normally (my take is always, but opinions may vary) a bug that needs to be reported / fixed.

Since it is the client that sent a wrong request, this should probably have been caught earlier on the server side as a 4xx client error. I am not sure if there is anything that needs to be fixed on the client side e.g., if it's still possible to trigger that error when WebSockets are working by clicking delete twice quickly before the UI refreshes (I tried but was not able to reproduce it after fixing the WebSockets).

Yes, it will notify user (send email) to re-review the pull request. The "cancel request for changes" also fits in this logic: you request yourself to re-review the pull request in order to cancel previous review results (request for changes).

As to the 500 error, OneDev uses websocket to update UI in many places. This simplies UI refreshing a lot, and yet proves to be pretty reliable as long as websocket works. I admit that there is a chance to click twice to cause this error before UI is refreshed. But in practice this rarelly happens. I will see if this problem is reported frequently (for now, only once), and decide if necessary to improve the UI rendering logic.

The "cancel request for changes" also fits in this logic: you request yourself to re-review the pull request in order to cancel previous review results (request for changes).

As exemplified by me filing this issue, my brain did not come to that conclusion on its own, despite staring at the interface for several minutes explicitly looking for it.

As to the 500 error, OneDev uses websocket to update UI in many places.

Regardless of the UI though, from a systems engineering perspective, the Client and the Server should independently work reliably and not possibly do something as terrible as indexing out of bounds if one goes rogue :) It's a security thing as well. So this 500 error results from a lack of validation that should be fixed on the server side and return a client 4xx error, regardless of whether the client-side UI is improved for the refreshing or not.

Here is another example of something that should not be a 500:

https://code.onedev.io/projects/onedev-manual/issues/2/activities ( linked from https://code.onedev.io/onedev/server/~issues/151 )

If this resource no longer exists, this should be a 404, not a 500. A 500 is an unexpected situation that needs to be fixed.

(Ideally, that one should be a 301 redirect to https://code.onedev.io/onedev/manual/~issues/2/activities so we can keep finding those linked resources)

As exemplified by me filing this issue, my brain did not come to that conclusion on its own, despite staring at the interface for several minutes explicitly looking for it

Will investigate to see if there is better approaches playing well with current concept...

egardless of the UI though, from a systems engineering perspective, the Client and the Server should independently work reliably and not possibly do something as terrible as indexing out of bounds if one goes rogue :) It's a security thing as well. So this 500 error results from a lack of validation that should be fixed on the server side and return a client 4xx error, regardless of whether the client-side UI is improved for the refreshing or not.

Various exceptions such as IndexOutOfBoundsException or NullPointerException may be thrown but unintended logic will never be executed. OneDev does not have front end / back end separation, with its use of Wicket, a web component framework. My design goal is to make the UI reliable, and if occasionally it enounters errors, enough information should be available to investigate the error.

For the obsolete url you mentioned, I did not make them auto redirected for code brevity as there are not many old OneDev instances in the wild.

OneDev does not have front end / back end separation, with its use of Wicket, a web component framework.

Either Wicket offers a way to validate the requests and return the appropriate HTTP status, or it (and OneDev using it) goes against the HTTP protocol standard.

https://www.w3.org/Protocols/rfc2616/rfc2616-sec6.html#sec6.1.1

• 4xx: Client Error - The request contains bad syntax or cannot be fulfilled
• 5xx: Server Error - The server failed to fulfill an apparently valid request

Possibly this is an issue we need to file with Wicket to make it easier to not return IndexOutOfBoundsException and NullPointerException but instead return a validated, more obvious explanation of what triggered an error (which should make it much easier to investigate as well).

As a C developer, seeing a NullPointerException or IndexOutOfBoundsException in what should be a stable production environment scares the $&!^ out of me ;)

For the obsolete url you mentioned, I did not make them auto redirected for code brevity as there are not many old OneDev instances in the wild.

Even if no re-direction are implemented, returning a 404 would be a lot better than a 500.

Not a wicket problem. I mean OneDev does not have back end service intended to be used by others, and I am a bit lazy on differenticating different cases to return different http status codes for these rare cases.